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

Create getJDK.sh to download the non-Jigsaw JDK 9 #68

Merged
merged 1 commit into from Jun 25, 2017
Merged

Create getJDK.sh to download the non-Jigsaw JDK 9 #68

merged 1 commit into from Jun 25, 2017

Conversation

lasombra
Copy link

This should fix issue #66

@neomatrix369
Copy link
Member

@lasombra Works on Ubuntu - good to merge, can I request a new PR where you extract out the common parts in both getJDK.sh and getJigsawJDK.sh and put them in a common script, source that common script in both getJDK.sh and getJigsawJDK.sh, this will help remove some duplicated code and make it also easier to understand both scripts - thanks.

@neomatrix369
Copy link
Member

@lasombra Can you also confirm if this has been tested on both MacOSX and Linux distros on your end?

@neomatrix369 neomatrix369 self-requested a review June 14, 2017 22:21
neomatrix369
neomatrix369 reviewed Jun 25, 2017
View reviewed changes
Copy link
Member

@neomatrix369 neomatrix369 left a comment

Choose a reason for hiding this comment

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

Thanks @lasombra for coming back with a response, apologies for not looking at it earlier

@neomatrix369
Copy link
Member

neomatrix369 commented Jun 25, 2017 •
edited

@lasombra Could you please reply to my prior comments and then we can proceed with this PR towards merging and closing.

@lasombra
Copy link
Author

@neomatrix369 I'm sorry, completely missed it. I rested on Mac OS X and a RHEL server.

@neomatrix369
Copy link
Member

Thats great

@neomatrix369 neomatrix369 merged commit 5b98978 into AdoptOpenJDK:master Jun 25, 2017
This was referenced Jun 25, 2017
Closed
Closed
@neomatrix369
Copy link
Member

No worries @lasombra I have merged it, closed the issues and also created a new enhancement issue #69, in case it peeks your interest I'll assign it to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neomatrix369 neomatrix369 neomatrix369 left review comments

Assignees

@lasombra lasombra

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lasombra @neomatrix369 @iweiss