[OPENJDK-3676] WIP: convert installing some things over to artifacts: #559
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmtd
force-pushed
the
ubi10-test-artifacts
branch
3 times, most recently
from
05f7c2b to
4551ad0
Compare
sefroberg
approved these changes
Feb 25, 2025
There was a problem hiding this comment.
Ok, after reading the artifacts doc I think I get it. I wonder if structuring the github repo to mimic the container image's structure might make this whole operation a lot more simple. Great progress though. For what you are attempting to do here, great improvements. Looks good to me.
The Maven provided in RHEL10 is Maven 3.9. Signed-off-by: Jonathan Dowland <[email protected]>
jmtd
force-pushed
the
ubi10-test-artifacts
branch
from
60bf706 to
09ec0c4
Compare
We currently move scripts into place by executing a configure.sh script, and changing ownership and permissions etc. However, in the majority of cases we could instead declare where scripts should be installed using artifacts:. The advantages are: smaller sources; more declarative; more uniform ownership (default root:root); no need to chown (set the permissions on the script in git directly). Also addresses OPENJDK-2814 (running user shouldn't own /opt/jboss/container) Signed-off-by: Jonathan Dowland <[email protected]>
jmtd
force-pushed
the
ubi10-test-artifacts
branch
from
09ec0c4 to
9b27cf4
Compare
jmtd
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP WIP WIP
Depends upon #562 .
This would resolve https://issues.redhat.com/browse/OPENJDK-2814 and https://issues.redhat.com/browse/OPENJDK-3674
The pattern used in our containers for copying files in, up to now, was to do so entirely inside a script
configure.sh, with the files to be copied in an adjacent directoryartifacts, normally with a local directory heirarchy mirroring the destination in the image, e.g.modules/jdk/21/artifacts/opt/jboss/container/openjdk/jdk/jvm-options. The script usually changed the ownership and/or permissions of the files after copying them.Cekit offers a different method, artifacts, which for some reason we overlooked using for this. Using this is normally shorter, by default sets the owner and group to root, and is more declarative, so makes the images easier to reason about. For the above example artifact, the diff to the configuration to copy it in would be
I've gone a step further in this PR and removed the deep directory heirarchies encoding the destination path, since that's now present in the YAML. Personally I find the deep directory heirarchies hard to work with.
Most of the
configure.shscripts have been removed, as all they did was the copying.The copied files are all now owner by root:root, which I prefer, it was always weird to me that the scripts could theoretically be modified by the running web application.
Cekit
artifactshandling does not change file permissions, so I've set the necessary permissions on the scripts in git itself.