Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to the latest OSGi JDBC specification #2017

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Dec 28, 2022

In the latest release of the JDBC specification there was a requirement added that datasources should promote what methods they implement. mssql-jdbc implements all methods and could therefore promote all.

This also migrates from the org.osgi.enterprise artifact to the more specific org.osgi.service.jdbc one and adds the official TCK test as part of the JUnit-Suite.

FYI @stbischof

Copy link

@stbischof stbischof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the properties and covering it bY osgi tck

@Jeffery-Wasty
Copy link
Member

Hi @laeubi,

We'll look into this, confirm whether these are all the changes we need to make spec, and get back to you. Thank you for your contribution.

@Jeffery-Wasty
Copy link
Member

Hi @laeubi,

I'm unable to find the specification you're referring to. Do you happen to have a link?

@lilgreenbird lilgreenbird added this to In progress in MSSQL JDBC via automation Jan 24, 2023
@Jeffery-Wasty
Copy link
Member

Jeffery-Wasty commented Jan 30, 2023

I see that OSGI compendium on maven has migrated to here: https://mvnrepository.com/artifact/org.osgi/osgi.cmpn, which is probably why we're still using version 5. We missed that the page had changed. However, this page only has up to version 7, is there a maven repository for compendium 8?

@laeubi
Copy link
Contributor Author

laeubi commented Jan 30, 2023

R8 do not has a "fat compendium jar" anymore but many small dedicated items and therefore uses

<dependency>
  <groupId>org.osgi</groupId>
  <artifactId>org.osgi.service.jdbc</artifactId>
  <version>${osgi.jdbc.version}</version>
  <scope>provided</scope>
</dependency>

now.

@Jeffery-Wasty
Copy link
Member

I see, thank you. And thanks for the quick reply!

@Jeffery-Wasty
Copy link
Member

Hi @laeubi,

The dependency changes are causing test failures, as seen below, coming from the org.eclipse.osgi dependency. I don't see this as a dependency of org.osgi.service.jdbc, what is this needed for?

@laeubi
Copy link
Contributor Author

laeubi commented Jan 31, 2023

@Jeffery-Wasty I'll try to take a look the next days, I think I might perform the upgrade in smaller steps to see whats going on here...

@laeubi
Copy link
Contributor Author

laeubi commented Feb 4, 2023

@Jeffery-Wasty I have now started with a very minimal change, that just upgrades to the new artifact location from central:

It would be great if you can take a look, probably this could even be merged already as a first step.

@laeubi
Copy link
Contributor Author

laeubi commented Feb 4, 2023

I created another one that do the actual code changes here:

with those it should be possible to see if that alone causes any issues

@laeubi
Copy link
Contributor Author

laeubi commented Feb 4, 2023

And finally we have the test execution added here:

@Jeffery-Wasty
Copy link
Member

Thanks @laeubi, we'll take a look at each of the PRs you have linked.

@Jeffery-Wasty
Copy link
Member

As stated in #2066, lets use this as the sole PR. If you're able to fix the test to not use Junit5 it can be included, otherwise we'll move forward without it.

@laeubi
Copy link
Contributor Author

laeubi commented Feb 9, 2023

I fear the test requires JUnit5

@laeubi
Copy link
Contributor Author

laeubi commented Feb 9, 2023

My only idea is that one probabbly can create a new module that only contains this single test ... but the repo seems not really a multi-module one... so for now then we should go without the test.

@Jeffery-Wasty
Copy link
Member

I see, thank you for looking into this. If we end up being able to use Junit5 moving forward, we can revisit adding the test on, its always good to have additional testing. For now, if we can ask you to push a commit removing the test, we can review the PR and get it merged.

@laeubi laeubi force-pushed the upgrade_osgi_jdbc branch 2 times, most recently from 973961e to cdd04be Compare February 9, 2023 19:11
@Jeffery-Wasty Jeffery-Wasty self-requested a review February 9, 2023 19:11
@laeubi
Copy link
Contributor Author

laeubi commented Feb 9, 2023

Now everything should be cleaned up and rebased

@Jeffery-Wasty Jeffery-Wasty moved this from In progress to Under Peer Review in MSSQL JDBC Feb 9, 2023
@Jeffery-Wasty Jeffery-Wasty added this to the 12.3.0 milestone Feb 9, 2023
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Feb 10, 2023
tkyc
tkyc previously approved these changes Feb 10, 2023
@laeubi laeubi dismissed stale reviews from tkyc and Jeffery-Wasty via c135923 February 11, 2023 15:52
MSSQL JDBC automation moved this from Under Peer Review to In progress Feb 11, 2023
In the latest release of the JDBC specification there was a requirement
added that datasources should promote what methods they implement.
mssql-jdbc implements all methods and could therefore promote all.

This also migrates from the org.osgi.enterprise artifact to the more
specific org.osgi.service.jdbc one and adds the official TCK test as
part of the JUnit-Suite.
@lilgreenbird lilgreenbird moved this from In progress to Under Peer Review in MSSQL JDBC Feb 13, 2023
@Jeffery-Wasty Jeffery-Wasty merged commit bd01329 into microsoft:main Feb 15, 2023
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants