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.
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
Global image for external #5553
base: master
Are you sure you want to change the base?
Global image for external #5553
Changes from 22 commits
37d0c30
0ae0514
e415d58
5047445
3a1b749
9f3f031
520656f
5f88915
b028d4a
0a3f3db
b1e20d1
a3638b3
b47ef1e
26cd87f
cecf323
82b1c78
e36ff60
172a1f5
0839a27
673b223
849d2bd
b7f25af
14dd897
d8acc30
a1db200
e4b11c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is the point of this file? Seem it will never actually be used, as most external application tests vary so much, it does not have value. Drop it. It can be added in a later PR when and if it becomes needed.
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.
Sure. But before I do, please check: 5f88915#diff-3e2f7b162bce7417355d4fe0f18e6366b69e6abe5e73356f863274d3a4f9c04bR2 Those
generic_packages="git wget unzip tar curl"
are used in 99% of tests (see the rest of 5f88915 and search for -ubuntu_packages or -ubi_packages). I think such base really should be added. (note, this is not disagreeing with removal, just asking for brainstroming. I will remove it solemnly on this comment).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.
@smlambert Even if practically unused, I would liek to keep this file. Is is extremly helpful when new distribution is tried. It simply allows you to declare its specific dependencies on one place, and then you can easily pinpoint it. And if this file remains unused for future, then they can be tuned up for individual suites alter.
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 EXTERNAL_AQA_IMAGE needs to be set? If yes, should move to line 114 for both temurin and openj9 ? Looks like this is not needed?
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 unhappy workaround for a corner case I was unable to manage. The only correct sollution to this is described in middle of initial description of issue #5555
This triple-if is changing to much to be served by the methods in provider.sh (without adding similar triple if there).
If the EXTERNAL_AQA_IMAGE is set in this combination, the exit is called. If not (and that is the only expected case as you need to set three parameters out of any defaults to actually overwrite them by this last-of-three ifs the setup of EXTERNAL_AQA_IMAGE is ensuring the values detected later (which are no longer consistent with the set ones because of those ifs) are correct.
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.
To construct the EXTERNAL_AQA_IMAGE artificially and globaly, is quite a good idea, and I was playing with it a lot. The reason why I have not included it, are non deterministic OSes. Ubuntu was hardcoded, to be there for jdk-xy-temurin/openj9, because there is no record of ubuntu in name. From my limited knowledge, this is the only case where it is actually missing. Eg redhat java images still have ubi in name, and so on.
If there will be more usage of EXTERNAL_AQA_IMAGE then current baseurl, path, name, os variables, then I guess such a crossroad table would be necessary, but for now I decided to not implement it.
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 variable
packages
was removed, but still used here. This might be the reason whyRUN dnf install -y && dnf clean all
cannot catch any packages.If some core functions in this PR are needed in urgent, as Sophia suggested, we can extract this PR to only address the core functions, then work on non-core functions, e.g.
generice_packages
in other PRs, combining them all in this PR makes it easy to break current build. Thanks.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.
on line 512 this
os
variable is in Uppercase - which one should it be?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.
They have nothing in common. The OS was introduced 4 years ago, os was introduced 2 years ago. They both are local. I handle the same additional logic to both of them, as both share the value of current os. I can refactor to be both lowercase (As are other local variables). But I do not strong preffernece, nor do I want to add more complexity to this PR. Still Sharp eye you have. TY!
Both probably share that theirs authors are originally form Asia origin :)
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.
Will this whitespacing produce a consistent output?
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 had aligned itto that "====" format Shelly suggested. They provide as consistent output as possible. Do you have any more exact concern with them? Like being to long or simiarly?