Skip to content

Comments

Enable older Java versions#636

Merged
asfgit merged 1 commit intoapache:masterfrom
drigodwin:enable-ubuntu-java-versions
Apr 17, 2017
Merged

Enable older Java versions#636
asfgit merged 1 commit intoapache:masterfrom
drigodwin:enable-ubuntu-java-versions

Conversation

@drigodwin
Copy link
Member

@drigodwin drigodwin commented Apr 14, 2017

Versions of Java prior to 1.8 are not available by standard on Ubuntu 16.x+, this adds the repo for older versions required by some software.

Tested on CentOS / Ubuntu

public static String installJava(int version) {
Preconditions.checkArgument(version == 6 || version == 7 || version == 8, "Supported Java versions are 6, 7, or 8");
List<String> commands = new LinkedList<String>();
commands.add(ok(addOpenJDKPPK()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this only happen if the version is 6 or 7?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry possibly I mean the other way round so you can ignore me

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it matter if the repo is enabled for the other versions too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see any harm in it

public static String installJava(int version) {
Preconditions.checkArgument(version == 6 || version == 7 || version == 8, "Supported Java versions are 6, 7, or 8");
List<String> commands = new LinkedList<String>();
commands.add(ok(addOpenJDKPPK()));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you wrap this in a ifExecutableElse1 instead of ok?

Copy link
Member Author

@drigodwin drigodwin Apr 17, 2017

Choose a reason for hiding this comment

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

OK ignores if the command fails (if the OS isn't Ubuntu for instance) so this should be fine. It's the pattern used elsewhere for similar things - see the lines below.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough


/**
* Adds the PPA for OpenJDK for older JDK versions (7 and lower) required by some software (e.g. JBoss)
*/
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that is the right was of doing this. I'm wondering if it would be better to handle this case by case, i.e. in the blueprints themselves

Copy link
Member Author

@drigodwin drigodwin Apr 14, 2017

Choose a reason for hiding this comment

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

I would agree and this is the case in YAML based blueprints, but this is the pattern used elsewhere, see JMX or NSS

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough too, let's go for that now but I would like to see a better pattern implemented in the future (if possible)

Copy link
Contributor

@Graeme-Miller Graeme-Miller left a comment

Choose a reason for hiding this comment

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

lgtm

@asfgit asfgit merged commit b6c8566 into apache:master Apr 17, 2017
asfgit pushed a commit that referenced this pull request Apr 17, 2017
@drigodwin drigodwin deleted the enable-ubuntu-java-versions branch April 17, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants