Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Options for AdoptOpenJDK and Java 11 #869

Merged
merged 3 commits into from
May 31, 2020
Merged

Add Options for AdoptOpenJDK and Java 11 #869

merged 3 commits into from
May 31, 2020

Conversation

ecdye
Copy link
Contributor

@ecdye ecdye commented May 5, 2020 •
edited
Loading

Fixes #868
See Also #629

Signed-off-by: Ethan Dye [email protected]

@ecdye ecdye force-pushed the AdoptOpenJDK branch 8 times, most recently from e1e99e6 to 28a9801 Compare May 5, 2020 19:54
@ecdye
Copy link
Contributor Author

ecdye commented May 5, 2020

@mstormi Comments?

mstormi
mstormi reviewed May 6, 2020
View reviewed changes
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
functions/menu.bash Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@ecdye ecdye force-pushed the AdoptOpenJDK branch 2 times, most recently from f5c8b80 to 5c00d62 Compare May 6, 2020 18:05
@ecdye ecdye requested a review from mstormi May 7, 2020 15:29
mstormi
mstormi requested changes May 7, 2020
View reviewed changes
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
functions/menu.bash Outdated Show resolved Hide resolved
@ecdye ecdye force-pushed the AdoptOpenJDK branch 2 times, most recently from e9ff7bc to 61bdc19 Compare May 8, 2020 01:24
mstormi
mstormi requested changes May 8, 2020
View reviewed changes
functions/adoptopenjdk-java.bash Outdated Show resolved Hide resolved
mstormi
mstormi requested changes May 8, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@mstormi
Copy link
Contributor

mstormi commented May 8, 2020

We have a problem right now with latest Azul build, but I cannot easily fall back due to the way the code works like when it was a static download URL.
Could you add a whitelist feature (to Azul and Adopt) ?
openHABian install would need to skip new versions when they're not in the whitelist and fall back to latest that's in there.
(for Adopt/apt you would probably need to use apt-cache madison to find out valid releases.

@ecdye
Copy link
Contributor Author

ecdye commented May 9, 2020

@mstormi I don't think that's necessary. The current issue is with the lack of ability to query an specific float (i.e. hf or sf) I'm going to see if Azul can implement something to fix it, but for the moment the patch that you made should suffice to avoid the issue.

@ecdye ecdye requested a review from mstormi May 10, 2020 01:29
mstormi
mstormi requested changes May 10, 2020
View reviewed changes
functions/java-jre.bash Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@ecdye ecdye force-pushed the AdoptOpenJDK branch 2 times, most recently from 945c3c3 to 6209aa8 Compare May 10, 2020 21:40
@ecdye ecdye requested a review from mstormi May 12, 2020 17:39
mstormi
mstormi requested changes May 13, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
functions/java-jre.bash Outdated Show resolved Hide resolved
functions/java-jre.bash Outdated Show resolved Hide resolved
functions/menu.bash Outdated Show resolved Hide resolved
@ecdye
Copy link
Contributor Author

ecdye commented May 29, 2020

After all, stable it supposed to prevent breaking changes and broken code entering from our end not the upstream IMHO. By including the java install in the image than we already remove the weakest link in the install (or the link that we loose the most users from) as it will already be downloaded.

Stable should not be restrictive but rather protected from developer mistakes that break code.

@mstormi
Copy link
Contributor

mstormi commented May 29, 2020

but then do we not allow the user to change the java installation on stable?

Of course we should allow for updating if the user wants it, it never was my intention to restrict that.
Maybe display a whiptail warning and ask for proceeding (with default button no) but only when we're on stable ? else on master download right away
I just don't want to use latest (untested) Java but if someone wants to he can get it.

mstormi
mstormi requested changes May 29, 2020
View reviewed changes
Copy link
Contributor

@mstormi mstormi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I couldn't validate functionality is as we discussed. Will need to test with a live system.
One last to go please incorporate the download change as per my last comment then I guess we're set.

Please create another PR to merge into testbuild.

Next time, please only ever change code related to the issue you open the PR for, else we're heading into chaos fast.

Could you also write a short announcement including things users should know ?
Add to repo as a file called "NEWS". That'll popup on future starts.

Will publish a request to the community to test testbuild on Sunday or so on the forum.

Remember the harder the review cycles the better the outcome.
Thanks for your patience - this was good work.

functions/influxdb+grafana.bash Outdated Show resolved Hide resolved
functions/influxdb+grafana.bash Outdated Show resolved Hide resolved
functions/menu.bash Outdated Show resolved Hide resolved
@ecdye ecdye force-pushed the AdoptOpenJDK branch 2 times, most recently from fb1c2ab to 9fd3f58 Compare May 29, 2020 22:34
@ecdye
Copy link
Contributor Author

ecdye commented May 29, 2020

@mstormi one last time, is this what you wanted?

@ecdye ecdye force-pushed the AdoptOpenJDK branch 3 times, most recently from 76f76d0 to 931b4cb Compare May 30, 2020 01:19
@mstormi
Copy link
Contributor

mstormi commented May 30, 2020 •
edited
Loading

LGTM but now that I've merged #932 there's conflicting files, please resolve.

Please remove NEWS.md again (I took a copy, but it would overwrite news from other PRs).
See https://github.com/mstormi/openhabian/blob/config-interactive/docs/NEWSLOG.md to come.

And don't forget the testbuild PR please

@ecdye ecdye force-pushed the AdoptOpenJDK branch 4 times, most recently from 02b5e8d to 6e1e60e Compare May 31, 2020 04:07
@mstormi
Copy link
Contributor

mstormi commented May 31, 2020

Please don't beat me now but I have been wondering if we should really deploy your Java into the testbuild branch or rather use master instead.
Initially, I wanted to have that new branch to be on the safe side your patch does not break anyone's production even when he does change Java.
But now with #882 merged everybody to update openhabian-config should be put on stable and breaking master would not do much harm (hopefully so - if I don't overlook anything).
Starting to juggle with 3 branches will increase probability of users and ourselves to confuse stuff.

Are you ok with that?
Then please double-check this PR here and fix that minor conflict with .travis.yml.
I'll adapt the announcement post.

@mstormi
Copy link
Contributor

mstormi commented May 31, 2020

are you sure it'll work on x86, too ? Cannot test that myself.

@mstormi
Copy link
Contributor

mstormi commented May 31, 2020

@openhab/core-maintainers please check https://community.openhab.org/t/java-testdrive/99827 and let me know if you want any changes to that procedure and doc

@ecdye
Copy link
Contributor Author

ecdye commented May 31, 2020

I think master is fine, I was wondering the same hing last night when I updtated the testbuild PR to make the files be more accurate.

@ecdye
Copy link
Contributor Author

ecdye commented May 31, 2020

are you sure it'll work on x86, too ? Cannot test that myself.

It should, the apt repo provides it for all of the platforms. Afterall that is why we are testing this :)

@mstormi
Copy link
Contributor

mstormi commented May 31, 2020

Alright let's go live with master then.

@mstormi mstormi merged commit 6f886da into openhab:master May 31, 2020
@mstormi
Copy link
Contributor

mstormi commented May 31, 2020

Here we are. Thanks for your work and patience.
I listed and globally pinned the announcement.
Please keep having an eye on the forum's Development section.

@mstormi mstormi removed the awaiting feedback Waiting for additional information label May 31, 2020
mstormi pushed a commit to mstormi/openhabian that referenced this pull request Jun 10, 2020
@ecdye @mstormi
Add Options for AdoptOpenJDK and Java 11 (openhab#869)
e48ff03 
* Add Option for AdoptOpenJDK
  Fixes openhab#868
  See Also openhab#629
* Add Java 11 Support
* Prepare Java for inclusion in openHABian image

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Markus Storm <[email protected]>
mstormi pushed a commit to mstormi/openhabian that referenced this pull request Jun 10, 2020
@ecdye @mstormi
Add Options for AdoptOpenJDK and Java 11 (openhab#869)
fa16433 
* Add Option for AdoptOpenJDK
  Fixes openhab#868
  See Also openhab#629
* Add Java 11 Support
* Prepare Java for inclusion in openHABian image

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Markus Storm <[email protected]>
@ecdye ecdye deleted the AdoptOpenJDK branch August 2, 2020 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstormi mstormi mstormi requested changes

Assignees

@ecdye ecdye

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AdoptOpenJDK Option
4 participants
@ecdye @mstormi @wborn @cweitkamp