Skip to content

Upgrade to the latest OSGi JDBC specification#2017

Merged
Jeffery-Wasty merged 1 commit intomicrosoft:mainfrom
laeubi:upgrade_osgi_jdbc
Feb 15, 2023
Merged

Upgrade to the latest OSGi JDBC specification#2017
Jeffery-Wasty merged 1 commit intomicrosoft:mainfrom
laeubi:upgrade_osgi_jdbc

Conversation

@laeubi
Copy link
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Hi @laeubi,

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

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

laeubi commented Feb 4, 2023

And finally we have the test execution added here:

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

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

@Jeffery-Wasty
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

laeubi commented Feb 9, 2023

I fear the test requires JUnit5

@laeubi
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

laeubi commented Feb 9, 2023

Now everything should be cleaned up and rebased

@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
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/osgi/Activator.java Outdated
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants