-
-
Notifications
You must be signed in to change notification settings - Fork 230
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 #760 - updatecli changes to enable JDK updates when new JDK versions are published #761
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4a08cb2
Issue #760 - Change JDK configuration of updatecli to enable the auto…
tmousawNF d9ace13
Issue #760 - Update typo in the name of checkTemurinWIndowsCore2022Do…
tmousawNF 16b423a
Issue #760 - Update check-jdk.sh to support the new URL format for JD…
tmousawNF a6d52db
Issue #760 - Updates to JDK updatecli yaml configs to support hot fixes
tmousawNF 8b13a14
Issue #760 - Remove unnecessary sourceid on targets in jdk11.yaml config
tmousawNF f95d636
Merge branch 'master' into bugfix/760
tmousawNF b426cf9
- Remove support of "hotfix" versions
tmousawNF 0946da7
Make `values.temurin.yaml` POSIX compliant
tmousawNF 6738da4
Merge branch 'master' into bugfix/760
tmousawNF File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
It looks like this transformation works as expected:
as per the last builds.
However the targets do not benefit from this:
same build.
=> We might want to factorize all the transformers directly into the source (at least the pattern selection) so no need to maintain all of these across conditions and targets, WDYT?
=> Which raise the question: with only the fix on the architectures (which I would use with the long form everywhere so it is homogeneous and easier to get), shouldn't the version
11.0.22_7
be picked by the source without the combination of (changed regex + removing last digit with transformer to handle the specific aarch64 case)?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 see in #762 that the current source also picks
jdk-11.0.22+7.1
.This exotic schemes looks like a problem!
@tmousawNF @gounthar @MarkEWaite @timja
WDYT about keeping the current regexp for source, but add the transformer proposed in this PR to remove any additional number after the
+
?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.
This is exactly what I intended. When a hot fix is published, the docker image will be tagged without the hot fix portion (i.e.
11.0.22_7
instead of11.0.22_7.1
in this particular case) such that the previously published docker image is replaced with the newer docker image. However, the actual "JAVA_VERSION" we're building the docker images for will be based on the hot fix, so I think it is correct to update the "JAVA VERSION" to include the hot fix portion. Otherwise, if we had built the image for version11.0.22_7
, how would we trigger the docker image to build again to pick up the hot fix? It's possible I'm misunderstanding how this works, but my assumption is that the update of the "JAVA_VERSION" triggers the docker image to re-build. Please let me know if I have a misunderstanding here.Yes. This is a very practical suggestion. I'll work on an update to accomplish this.
If I understand your question, it is true that version
11.0.22_7
will be picked up without the change when the11.0.22_7
version is published. However, as I stated in my previous response, when11.0.22_7.1
is published, without the changed regex how is the hot fix built into a docker image? Again, I could be misunderstanding what triggers the build of the docker image and, if so, please let me know.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.
Actually, now that I try to build with these suggested changes, I see that this is an issue because the
JAVA_VERSION
is pulling the docker image. How do we ensure that docker images get built when hot fixes are published?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.
Hey,
Small suggestion would it make sense to also pin using the docker image digest something like
eclipse-temurin:11.0.22_7-jdk-alpine@sha256xxxxx
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.
What @gounthar said: the trick here is that not all releases from Temurin correspond to a full set of platform even for Docker images.
Today:
11.0.22+7
(11.0.22_7
) today to be picked because it has published images for all of our platformsHowever we can still reconsider this choice in the future of course but a full policy with examples must be written (which makes it hard to be exhaustive given each baseline of Temurin JDK seems to have their own schemes : not easy).
If we go back to Temurin installers inside our images, instead of using multi stage builds from the Temurin Docker images (as discussed during former Jenkins Platform SIG meetings) we would have to revisit this of course.
I believe we could benefit from directly retrieving the package through their API (which would ensure the proper version for each platform) as I wrote in this updatecli RFE: updatecli/updatecli#1733
Does it make sense?
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.
No problem @olblak I believe the discussion is mandatory here to ensure the best contribution from @tmousawNF and it also benefits the community! Thanks for weighing in! I believe it is another use case for updatecli/updatecli#1733 (which might interest you)
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.
With respect to @gounthar's comment:
I found an example of where there was a "hotfix" that included new docker images (see https://github.com/adoptium/temurin17-binaries/releases/tag/jdk-17.0.9%2B9.1).
I'm going to suggest that I will remove the detection of the "hotfix" versions and only detect the major versions. In the meantime, we are no worse off than before since the "hotfix" causes issues in the existing config file (just try to run the
jdk11.yaml
config today).I have also yet to factorize all the transformers. I will work on that now.
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 remember this one, true!
There was no associated Docker image: https://hub.docker.com/_/eclipse-temurin/tags?page=1&name=17.0.9_9.1 (compared to https://hub.docker.com/_/eclipse-temurin/tags?page=1&name=17.0.9_9).
I wonder about their packaging policy in these cases: do they override the existing images with the hotfix content?
Anyway, thanks for your work: it alrerady fixes a LOT of cases and improves the situation so let's roll once you're finished with the refacto!
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 believe what happens is that the "hotfix" is published with the same tag and overwrites the existing docker image (i.e. becomes a newer version of tag
17.0.9_9
). You would then need to pull the base image again and rebuild.Thanks for the active discussion! It helps greatly (especially since I am new to
dcker-agent
andupdatecli
).Since I removed the portion looking for "hotfix" versions, I was able to remove the
findsubmatch
part of the transformations in the conditions of thejdk*.yaml
config files. I created a new filevalues.temurin.yaml
and added an extra--values ./updatecli/values.temurin.yaml
to.github/workflows/updatecli.yaml
. I considered possibly adding the new value tovalues.github-action.yaml
, but that would have meant non-GitHub actions related values in that file. One option is to rename that file and merge the two values files. I'm happy to do that if the consensus is that is better.