-
Notifications
You must be signed in to change notification settings - Fork 500
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
allow newer java versions (LTS and current), correct interactive logic #6983
Conversation
…condition per Leonid
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'm not so sure if makes sense to support Java 14. Details inline.
scripts/installer/install.py
Outdated
if not re.search("1.8", java_version): | ||
sys.exit("Dataverse requires Java 1.8. Please install it, or make sure it's in your PATH, and try again") | ||
if not re.search('(1.8|11|14)', java_version): | ||
sys.exit("Dataverse requires Java 1.8, 11 or 14. Please make sure it's in your PATH, and try again.") |
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.
Supporting Java 14 feels a little arbitrary. It's not an LTS release. It just happens to be the current release. My concern is that if we really want to support the latest release of Java, we're going to have to edit this line every six months. I'd be happier with just putting the next LTS release in there, Java 17, which is due out September 2021. Or we could remove Java 14.
Here's a table from Wikipedia that shows all the recent Java releases:
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.
@donsizemore we talked about this a bit in Slack. Can you please remove Java 14 and only support 8 and 11? 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.
Let's only allow LTS releases for now: Java 8 and 11.
scripts/installer/install.py
Outdated
if not re.search("1.8", java_version): | ||
sys.exit("Dataverse requires Java 1.8. Please install it, or make sure it's in your PATH, and try again") | ||
if not re.search('(1.8|11|14)', java_version): | ||
sys.exit("Dataverse requires Java 1.8, 11 or 14. Please make sure it's in your PATH, and try again.") |
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.
@donsizemore we talked about this a bit in Slack. Can you please remove Java 14 and only support 8 and 11? Thanks!
What this PR does / why we need it:
In testing/using dataverse-ansible, @pallinger correctly pointed out that 1) install.py requires openjdk 1.8, and 2) somehow when openjdk-1.8.0 is specified install.py carries on in ansible without complaint, but when openjdk-11 is specified it hangs waiting for a password, when --noninteractive is passed.
This PR modifies install.py to allow OpenJDK 1.8 (long-term RedHat support), 11 (OpenJDK LTS) and 14 (current OpenJDK, not recommended), and corrects the noninteractive password logic per @landreev
Which issue(s) this PR closes:
Closes #6982
Special notes for your reviewer: none come to mind
Suggestions on how to test this: re-run install.py with and without --noninteractive, while openjdks 1.8.0, 11 and maybe 14 are set as default
Does this PR introduce a user interface change? If mockups are available, please link/include them here: no
Is there a release notes update needed for this change?: no
Additional documentation: none.
This can definitely wait until after the DCM.