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.

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

Support AdoptOpenJDK #370

Merged
merged 10 commits into from
Feb 13, 2020
Merged

Support AdoptOpenJDK #370

merged 10 commits into from
Feb 13, 2020

Conversation

timdeluxe
Copy link
Contributor

Since Oracle changed their licensing policies, alternatives are becoming more and more important.

The OpenJDK packages bundled with the different Linux operating systems are sometimes enough, but often you need specific versions, which are not available in the package management of the OS. Here AdoptOpenJDK comes to the stage, providing binary distributions in different archive formats, which are ready to use after simple extracting.

With this PR i provide code very similar to the oracle define for AdoptOpenJDK. I also changed documentation and added spec and acceptance tests, which run without errors.

Be aware that path and file naming in the AdoptOpenJDK world is somewhat crazy and i needed to implement a few tricks and tweaks to cover that.

The only thing i did not cover from the contributing guidelines were the commit messages, but maybe you can fix that by doing a squash merge combined with a good commit message.

If you have improvements or concerns, let me know. Otherwise we would be happy with a merge, because we need that feature in our environment (currently we use my github fork). Thanks!

@timdeluxe
Copy link
Contributor Author

Yes, i saw the failing test and i am going to fix that, sorry. (I just noticed that i missed that test before pushing)

@timdeluxe
Copy link
Contributor Author

As you can see the tests are now green.

Hint: I needed to rewrite the ensure_resources, due to unresolved bug https://tickets.puppetlabs.com/browse/MODULES-1323
This maybe also needed for the oracle part - it just was not noticed, because the acceptance tests for oracle are disabled by default because of often changing url hashes...

Let me know if i can do anything else for this PR.

@fraenki
Copy link
Contributor

fraenki commented Jul 8, 2019

@timdeluxe Did you already create a ticket in Puppetlabs' JIRA? (see https://github.com/puppetlabs/puppetlabs-java/blob/master/CONTRIBUTING.md) This should make it easier to get this merged. :)

@fraenki
Copy link
Contributor

fraenki commented Jul 8, 2019

@timdeluxe One more thing... could you possibly incoorporate the changes from #348 and #349 so that both defined types provide the same features? :) (This would save us another PR.)

@fraenki
Copy link
Contributor

fraenki commented Jul 8, 2019

@timdeluxe I've noticed an incompatibility: instead of using $java_se you've renamed the variable to $java. I'd suggest to stick with the $java_se name, because this way it would be pretty easy to replace a Oracle JDK with AdoptOpenJDK (or vice versa).

@fraenki
Copy link
Contributor

fraenki commented Jul 8, 2019

@timdeluxe I've fixed another compatibility issue by re-adding $url_hash (although it is unused for AdoptOpenJDK). Now both defined types are pretty much interchangeable – one can easily switch between Oracle JDK and AdoptOpenJDK. :)

Here's the list of changes I've applied on top of your PR, in case you want to incooperate them:
https://github.com/fraenki/puppetlabs-java/commits/adoptopenjdk
(I'd also volunteer to submit a new PR which includes all changes, if you don't mind.)

@timdeluxe
Copy link
Contributor Author

@fraenki

No ticket in Jira yet, i missed that. Question is if i still should do it or now you go ahead with it, since you put stuff on top already?

I know that i renamed java_se and removed url_hash. I did that on purpose. I am not a big friend of having them back just for the sake of being compatible to Oracle. One needs to adapt code anyway and then it should not hurt much to fix these two parameters. I find it incorrect and confusing to have them back.
Feel free to submit another PR, but we need to discuss about the parameters!

I agree with incorporating #348 and #349, but i will wait for your answer before doing anything. I also noticed wrong comments in line 200 and 211, those need to be fixed. And finally data types for the parameters would be good (as we noticed in our setup) - i am jungling with string and integer in the code as you can see. Maybe it is wise to have all the version stuff given as strings and convert them to integer whereever needed?

include ::archive

# validate java Standard Edition to download
if $java !~ /(jre|jdk)/ {
Copy link

@smasa90 smasa90 Jul 10, 2019

Choose a reason for hiding this comment

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

You can work with types to avoid this test eg. Enum['jdk', 'jre'] = 'jdk' on define parameters declaration
https://puppet.com/docs/puppet/6.6/lang_data_type.html

@fraenki
Copy link
Contributor

fraenki commented Aug 4, 2019

@timdeluxe I've spent some time with your PR and my additions in a production environment. I have to admit that I really enjoyed to be able to quickly switch between Oracle JDK and AdoptOpen JDK. We had to do this multiple times due to JDK incompatibilities, it was quite comfortable. ❤️

No ticket in Jira yet, i missed that. Question is if i still should do it or now you go ahead with it, since you put stuff on top already?

Hm, maybe it's OK to continue with this PR, the discussion might prove to be useful for a review.

I know that i renamed java_se and removed url_hash. I did that on purpose. I am not a big friend of having them back just for the sake of being compatible to Oracle.

I agree to remove the $url_hash parameter, because it's useless for AdoptOpen JDK. But renaming the $java_se parameter doesn't seem to improve anything, TBH. So I'd really favor keeping at least this parameter compatible, because it's a major one (thus it would be much harder to workaround this incompatibility in external code).

Besides that this would make it possible to deduplicate the oracle/adopt code in the future, which would be much harder otherwise.

I agree with incorporating #348 and #349

Thanks, very appreciated!

And finally data types for the parameters would be good (as we noticed in our setup) - i am jungling with string and integer in the code as you can see. Maybe it is wise to have all the version stuff given as strings and convert them to integer whereever needed?

Sure, but I'd suggest to implement this in a 2nd PR after this one got merged.

@sheenaajay
Copy link
Contributor

Thanks, @timdeluxe for submitting the PR. Could you please do the rebase and resolve the conflicts. Thank you.

@timdeluxe timdeluxe requested a review from a team as a code owner January 16, 2020 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants