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

PAYARA-2263 per-jdk version domain options #2585

Merged
merged 25 commits into from Apr 10, 2018

Conversation

@lprimak
Copy link
Contributor

lprimak commented Mar 28, 2018

Ability to enable / disable JDK options based on JDK version pattern

@lprimak

This comment has been minimized.

Copy link
Contributor Author

lprimak commented Mar 28, 2018

jenkins test

@payara-ci

This comment has been minimized.

Copy link
Contributor

payara-ci commented Mar 29, 2018

Quick build and test passed!

@lprimak lprimak requested a review from jGauravGupta Mar 29, 2018

@lprimak lprimak changed the title PAYARA-2263 per jdk version domain options PAYARA-2263 per-jdk version domain options Mar 30, 2018

@lprimak

This comment has been minimized.

Copy link
Contributor Author

lprimak commented Mar 30, 2018

jenkins test

@payara-ci

This comment has been minimized.

Copy link
Contributor

payara-ci commented Mar 30, 2018

Quick build and test passed!

@Pandrex247
Copy link
Member

Pandrex247 left a comment

Looks good, just some formatting quibbles :)

ar.getExtraProperties().put("leafList", getEntity());
if (isJvmOptions) {
ar.getExtraProperties().put("leafList", getEntity().stream().map(JvmOption::new)
.map(option -> ImmutableMap.of("minVersion", option.minVersion.isPresent()? option.minVersion.get().toString() : "",

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Please put a space before your '?'s
option.minVersion.isPresent() ? option.minVersion.get().toString()

@@ -334,7 +350,7 @@ private String getErrorMessage(Map<String, String> data, ActionReport ar) {
results.put(key, entry.getValue());
} else {
// options.append(sep).append(escapeOptionPart(entry.getKey()));
options.append(sep).append(entry.getKey());
options.append(sep).append(removeVersioning? new JvmOption(key).option: key);

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Space
removeVersioning ? new

public final Optional<JDK.Version> minVersion;
public final Optional<JDK.Version> maxVersion;

private static final Pattern PATTERN = Pattern.compile("^\\[(.*)\\|(.*)\\](.*)");

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Would be nice to have a comment describing what the pattern should match :)

This comment has been minimized.

Copy link
@lprimak

lprimak Apr 10, 2018

Author Contributor

Fixed

if (!minVersion.isPresent() && !maxVersion.isPresent()) {
return option;
}
return String.format("[%s|%s]%s", minVersion.isPresent()? minVersion.get(): "", maxVersion.isPresent()? maxVersion.get(): "", option);

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Spaces bitte
minVersion.isPresent() ? minVersion.get() : ""

This comment has been minimized.

Copy link
@lprimak

lprimak Apr 10, 2018

Author Contributor

Not sure if I agree, but I really don't care that much.
Question? AnswerA : AnswrB vs.
Question ? AnswerA : AnswerB

I think Question mark belongs together with the question itself.
Makes sense?

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 10, 2018

Member

Depends if you see the ? as an operator or not.
I see it as an operator, in a similar vein to + and =.

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 10, 2018

Member

We also use a space everywhere else we use ternary operators.

This comment has been minimized.

Copy link
@arjantijms

arjantijms Dec 7, 2018

Member

I know this an ancient thread, but just to add to this; there's a Sun code convention for it. Not sure I like it so much, but I've learned to just accept it ;)

private final Optional<Integer> update;

private Version(String string) {
String[] split = string.split("[\\._u\\-]+");

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Again, would be nice to have a comment describing what this should split on :)
Just helps with quick readability.

This comment has been minimized.

Copy link
@lprimak

lprimak Apr 10, 2018

Author Contributor

Fixed

public boolean newerOrEquals(JDK version) {
return newerThan(version) || equals(version);
public static Version getVersion(String string) {
if (string != null && string.matches("([0-9]+[\\._u\\-]+)*[0-9]+")) {

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Wanted: Descriptive comment :)

This comment has been minimized.

Copy link
@lprimak

lprimak Apr 10, 2018

Author Contributor

Fixed

if (minVersion.isPresent()) {
correctJDK = JDK_VERSION.newerOrEquals(minVersion.get());
}
if (correctJDK == true && maxVersion.isPresent()) {

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Do you need the == true? Isn't this just correctJDK && maxVersion.isPresent()?

This comment has been minimized.

Copy link
@lprimak

lprimak Apr 10, 2018

Author Contributor

Fixed

bag1.setJvmOptions(jvmopts);
int now = jvmopts.size();
if (removed) {
part.setMessage(lsm.getString("deleted.message", (orig-now)));

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Space
orig - now

payload.put(TARGET, target);
for (Map<String, Object> oneRow : options) {
String jvmOptionUnescaped = new JvmOption((String)oneRow.get(JVM_OPTION),
(String)oneRow.get(MIN_VERSION), (String)oneRow.get(MAX_VERSION)).toString();

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Apr 9, 2018

Member

Space between cast class and method please.
(String) oneRow.get(MIN_VERSION)

Show resolved Hide resolved ...launcher/src/main/java/com/sun/enterprise/admin/launcher/GFLauncher.java Outdated

arjantijms and others added some commits Apr 9, 2018

@lprimak

This comment has been minimized.

Copy link
Contributor Author

lprimak commented Apr 10, 2018

All fixed up

@lprimak

This comment has been minimized.

Copy link
Contributor Author

lprimak commented Apr 10, 2018

jenkins test

@payara-ci

This comment has been minimized.

Copy link
Contributor

payara-ci commented Apr 10, 2018

Quick build and test passed!

@lprimak lprimak merged commit 6d023e5 into payara:master Apr 10, 2018

3 checks passed

Payara Quick Build Payara quick build passed!
Details
Payara Quick Build and Test Quick build and test passed!
Details
Payara Quick Test Payara quick tests passed!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.