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
Add conditionals for Rhel9 in common role #3802
base: master
Are you sure you want to change the base?
Add conditionals for Rhel9 in common role #3802
Changes from 2 commits
307ef6f
885e859
a017ee6
9d879c2
a9fa00c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 54 in ansible/playbooks/AdoptOpenJDK_Unix_Playbook/roles/Common/tasks/RedHat.yml
GitHub Actions / Ansible Lint
yaml[trailing-spaces]
Check failure on line 54 in ansible/playbooks/AdoptOpenJDK_Unix_Playbook/roles/Common/tasks/RedHat.yml
GitHub Actions / Yamllint
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.
We probably need to go through and sanitise some of these names. This one should probably be
Additional_Build_Tools_RHEL6_RHEL7
now but that doesn't need to be changed in this PRThere 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.
Are these 3 cases still what we want?
From
infrastructure/ansible/playbooks/AdoptOpenJDK_Unix_Playbook/roles/Common/vars/RedHat.yml
Line 95 in 894d848
What is java 7 and 8 used for?
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.
@AdamBrousseau @sxa , I am not sure. Should we bump it to 17 or 21?
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.
As far as I can tell it's been there since the beginning. Java is needed for 2 purposes that I know of.
Since Jenkins now requires jdk17 to run agents, and jdk11 was required 2 years ago, I don't think leaving this at java8 for jenkins connections is necessary.
For bootjdk requirement, the build scripts will pull the needed jdk during the build process so having a permanent install of 8 is not necessary in my opinion.
I would want an opinion from Adopt/@sxa before we remove it completely though.
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.
As far as the boot JDK is concerned I don't want the builds pulling it down dynamically in most cases - that's mostly just a fallback mechanism. Other than pulling down the source, the builds should be able to run without network access.
I suspect those java-1.8.0* packages are no longer required, at least by Temurin, so it would likely be safe to remove them.
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.
@AdamBrousseau , @sxa ,
Since stewart is of opinion not to pull down the bootjdks by the jobs.And java8 is not required, Should we remove the installing and setting up default java part in this role or should we maybe bump it to jdk17 or 21?
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.
Yes I think that should be safe. It will hopefully be obvious if some part of the process requires it, in which case it's easy to back it out again :-)
Can the 1.7 version be installed from the RHEL9 repositories?
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.
@sxa is there another part of the PB installing java for use by the jenkins agent that Adopt is running?
We have an internal Semeru role that installs 21 for this.
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.
Yep. The adoptopenjdk_install role: .
infrastructure/ansible/playbooks/AdoptOpenJDK_Unix_Playbook/main.yml
Line 131 in 028773c
It's extracting as a tarball and doesn't set any default on the system to point to it, so the Jenkins agent is pointed directly to it under /usr/lub/jvm when it's started
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.
Since we still have the reference to
Java_RHEL8
above I believe this should not be removed