-
Notifications
You must be signed in to change notification settings - Fork 164
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
GH-4401 switch to new config vocabulary as the default #4699
GH-4401 switch to new config vocabulary as the default #4699
Conversation
037c218
to
37a3902
Compare
5158d2d
to
1d5e992
Compare
@hmottestad I'm getting a build failure in the osgi-runtime - I seem to recall you recently had a similar issue? |
Looks like there was a fix for the osgi issue on main which is not yet synced onto the develop branch. Doing a manual branch sync. |
Doing a small fix in the package-assembly github action - see #4746 |
- except when useLegacy property is set
69d6fda
to
fa9bd00
Compare
I'm having a bit of second thoughts about changing up the vocabulary. I know that we don't own the domain names anymore, but that doesn't really affect our code. I had put some effort into making the new and old vocabulary work together in 4.3, but it still managed to break the sail templates used in the workbench. And we don't have a single test for the workbench as far as I can tell. It's a bit worrying that we will have to do a lot of manual verification and hope we've thought of everything. And that also goes for everyone building on top of RDF4J, for instance GraphDB and Halyard. |
I'm planning to run some further manual tests and report those here. I've also, as part of this PR, significantly improved test coverage and added better support for mixed vocab usage as well as the choice to stick completely with legacy vocab for now (though I don't recommend that). Workbench test coverage is a problem, but not to the extent that I want to cancel this improvement. The breakage in 4.3 was unfortunate but those problems have been addressed, and this time we are doing this as part of a major release. |
Test Notesdefault behavior
useLegacyConfig flag enabled
|
Writing configs will still use old vocabulary in legacy mode, but reading will accept both vocabularies (preferring legacy, falling back to new if present)
d4e827d
to
ebdc4b8
Compare
Given the above and the test results I shared, could I get another review over this? If there's additional things you'd like to see tested (but don't have the time to do yourself) let me know and I can take a look at it over the weekend. If not, I would consider this done and we can merge it into develop. |
I'll try to find some time to look it over again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on my laptop a bit and didn't manage to break anything. So I think this is good to go.
Maybe it's time for another milestone build after this is merged.
GitHub issue resolved: #4401
Briefly describe the changes proposed in this PR:
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resources
to format from the command line)