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

CUSTOM-142 Delete JVM options with min max versions. #4409

Merged
merged 1 commit into from Jan 10, 2020

Conversation

@rdebusscher
Copy link
Contributor

rdebusscher commented Jan 6, 2020

This is a bug fix

When you try to delete a JVM option which has defined a min and/or max version, the option cannot be found (and thus not deleted).

This PR fixes the delete logic of the JVM Options.

Important Info

No Unit Test created due to proxy requirements of JavaConfig within org.jvnet.hk2.config.ConfigSupport#apply(org.jvnet.hk2.config.SingleConfigCode<T>, T)

Testing

Testing Performed

./asadmin start-domain
./asadmin create-jvm-options '[1.8.123|1.8.231]-DtestOption'
./asadmin delete-jvm-options '[1.8.123|1.8.231]-DtestOption'

  • Quicklook

Testing Environment

Maven 3.5.4
JDK 1.8u181

@rdebusscher rdebusscher requested review from dmatej, Cousjava and MeroRai Jan 6, 2020
@rdebusscher rdebusscher self-assigned this Jan 6, 2020
@dmatej
dmatej approved these changes Jan 9, 2020
@@ -162,7 +162,8 @@ private void deleteX(final JvmOptionBag bag, final List<String> toRemove, final
SingleConfigCode<JvmOptionBag> scc = (JvmOptionBag bag1) -> {
List<String> jvmopts = new ArrayList<>(bag1.getJvmRawOptions());
int orig = jvmopts.size();
boolean removed = jvmopts.removeIf(option -> toRemove.contains(new JvmOption(option).option));
// using new JvmOption(option).toString() (instead op option directly) to make sure the correct formatting is applied.
boolean removed = jvmopts.removeIf(option -> toRemove.contains(new JvmOption(option).toString()));

This comment has been minimized.

Copy link
@dmatej

dmatej Jan 9, 2020

Contributor

This method has expectations ... it expects that formatting is done in option.toString() and it's result will be same as strings in bag1.getJvmRawOptions().
Seems fragile to me,

getJvmRawOptions does not have javadoc, but it looks it is the content of the XML element.
JvmOption.toString prints also minVersion and maxVersion, seems it is really symetric at this moment ... ok.

@dmatej

This comment has been minimized.

Copy link
Contributor

dmatej commented Jan 9, 2020

Jenkins test please

@dmatej dmatej changed the title Delete JVM options with min max versions. CUSTOM-142. CUSTOM-142 Delete JVM options with min max versions. Jan 9, 2020
@MattGill98 MattGill98 merged commit 7002f7a into payara:master Jan 10, 2020
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
@rdebusscher rdebusscher deleted the rdebusscher:CUSTCOM-142 branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.