-
Notifications
You must be signed in to change notification settings - Fork 34
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
Issue 373, v2: Upgrade rdf-toolkit and Java #395
Conversation
The landing page for the setup-java action states the following: > NOTE: Adopt OpenJDK got moved to Eclipse Temurin and won't be updated > anymore. It is highly recommended to migrate workflows from adopt to > temurin to keep receiving software and security updates. See more > details in the Good-bye AdoptOpenJDK post. References: * https://github.com/actions/setup-java/#supported-distributions * https://blog.adoptopenjdk.net/2021/08/goodbye-adoptopenjdk-hello-adoptium/ Signed-off-by: Alex Nelson <[email protected]> (cherry picked from commit 2b15709)
With the current version of `rdf-toolkit-action`, this is expected to cause a CI failure due to an `rdf-toolkit.jar` incompatibility. A release coming soon for `rdf-toolkit-action` will update the `.jar` file used. Signed-off-by: Alex Nelson <[email protected]> (cherry picked from commit 282468f)
References: * #373 Signed-off-by: Alex Nelson <[email protected]>
The version of rdf-toolkit run by the pre-commit hook now handles an RDF list differently than the prior version of rdf-toolkit. This commit is a mechanically-produced result. References: * #373 Signed-off-by: Alex Nelson <[email protected]> (cherry picked from commit ab0aff8)
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.
The update changes all ordered lists into sets of IRIs. The application can only select one set element.
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.
If I'm correct, the wget on ...rdf-toolkit-build/lastSuccessfulBuild/...
might pull a new version in the future. This demands the new rdf-toolkit version to be backwards compatible with the current one to prevent the tool to potentially crash.
At the same time, constraining the toolkit with a SHA512 to one single version only, prevent new versions to be applied, and might make the tool to stall for different builds.
I assume this to be the intended behaviour.
@plbt5 , you're right that this was the behavior I intended to encode. This download strategy hasn't changed since CASE's initial implementation of it. However, you've made me realize that it assumes stability in the download, and that assumption is incorrect. The hard-coded SHA-512 "Pins" the expected JAR file to today's release. So, if a new release happens, the download will fail to verify. I'll push another patch to revise the strategy to use CASE's redistribution rather than retrieving the original file. I think a preferable remediation would be accessing a version somehow "Pinned" upstream, but that's out of scope of this PR. |
This prevents a potential build failure scenario where rdf-toolkit issues a release and suddenly all CI pinned to 1.11.0's SHA-512 fails. References: * #373 Reported-by: Paul Brandt <[email protected]> Signed-off-by: Alex Nelson <[email protected]>
References: * #373 Signed-off-by: Alex Nelson <[email protected]>
I'm not quite sure I follow this remark - the syntactic style of the ordered lists is changed, but they have not been converted into sets. |
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.
Quite so!
Closing as superseded by PR 398. |
This Pull Request will resolve all requirements of Issue 373.
This PR adjusts the strategy of v1, PR 386, due to issues having been found when running a normalized file through
rdf-toolkit
multiple times. Issue documentation is on CASE-Examples PR 75.pre-commit
usage is removed from v2 of resolving Issue 373.Review steps taken: