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

GEODE-5830: Fix NONE enum for SSL configuration #2581

Merged
merged 5 commits into from Oct 12, 2018

Conversation

jujoramos
Copy link
Contributor

GEODE-5830: Fix NONE enum for SSL configuration

The SecurableCommunicationChannel.NONE enum instance now references
the correct constant string, none, instead of NO_COMPONENT.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

The `SecurableCommunicationChannel.NONE` enum instance now references
the correct constant string, `none`, instead of `NO_COMPONENT`.
@@ -25,25 +27,14 @@
WEB(SecurableCommunicationChannels.WEB),
GATEWAY(SecurableCommunicationChannels.GATEWAY),
LOCATOR(SecurableCommunicationChannels.LOCATOR),
NONE("NO_COMPONENT");
NONE("none");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be made a constant, SecurableCommunicationChannels.NONE, to be consistent with the other declarations.
I think I was wrong about this -- however, we have the other channels supported as a comma-separated list, and I don't think it makes sense to have none included as a member of that list.

Copy link
Contributor

@galen-pivotal galen-pivotal left a comment

Choose a reason for hiding this comment

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

This still fails the test from the GEODE ticket:

  @Test
  public void test() {
    final Properties properties = new Properties();
    properties.setProperty(ConfigurationProperties.SSL_ENABLED_COMPONENTS, "none");
    try (final Cache cache = new CacheFactory(properties).create()) {
    }

It should pass this test. Also, "none" should probably not work in conjunction with other components.

Another option (since no one actually seems to be using "none" as it doesn't work) would be to remove the mention of it from the docs and default to no components like we do now.

@jujoramos
Copy link
Contributor Author

Agreed @galen-pivotal, I'll remove the none constant altogether and update the docs + tests.
Cheers.

The 'NONE' enum constant doesn't work and it's not used, so it's
entirely removed by this commit. Updated documentation and tests.
Last failure was not related to this PR's changes, adding
an empty commit to launch the CI from scratch.
Copy link
Contributor

@galen-pivotal galen-pivotal left a comment

Choose a reason for hiding this comment

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

Good cleanup and fix!

Per style guide, we need braces around all if statements. I left a comment inline. Apart from that, everything looks good.

}
if (ArrayUtils.contains(distributionConfig.getSecurableCommunicationChannels(),
SecurableCommunicationChannel.ALL)) {
SecurableCommunicationChannel.ALL))
Copy link
Contributor

Choose a reason for hiding this comment

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

Good cleanup here.

However, per the style guide, "Always use braces, even around one-line if, else and other control statements."


## <a id="ssl_configuration_properties" class="no-quick-link"></a>SSL Configuration Properties

You can use <%=vars.product_name%> configuration properties to enable or disable SSL, to identify SSL ciphers and
protocols, and to provide the location and credentials for key and trust stores.

<dt>**ssl-enabled-components**</dt>
<dd>List of components for which to enable SSL. Component list can be "all" or a comma-separated list of components.</dd>
<dd>List of components for which to enable SSL. Component list can be "" (disable SSL), "all" or a comma-separated list of components.</dd>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suuuper minor grammatical suggestion: put a comma after "all", so that there are clearly three commma-separated options.

@galen-pivotal
Copy link
Contributor

@jujoramos if you want, I'm happy to add the braces and merge. Just figured it might be helpful to get feedback on the style guide.

@jujoramos
Copy link
Contributor Author

Hello @galen-pivotal,

I've made the changes, thanks.
Regarding the code style guide, I was aware of it but I keep seeing one-line control blocks without brackets in new commits, so I assumed that the style guide changed and never went a checked the link again, sorry about that :-). It'd be worth modifying the spotless plugin to enforce this rule, BTW.
I'll wait for the CI to finish and I'll merge the changes afterwards, thanks again!.
Cheers.

Last failure was not related to this PR's changes, adding
an empty commit to launch the CI from scratch.
@jujoramos jujoramos merged commit 16cd4ae into apache:develop Oct 12, 2018
@jujoramos jujoramos deleted the feature/GEODE-5830 branch May 24, 2019 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants