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

empty properties treated as null #201

Merged

Conversation

azatkhakimov
Copy link
Contributor

@azatkhakimov azatkhakimov commented Apr 10, 2020

No description provided.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Apr 10, 2020

CLA assistant check
All committers have signed the CLA.

@azatkhakimov azatkhakimov deleted the bugfix-197-treatEmptyStringAsNull branch April 10, 2020 14:01
@azatkhakimov azatkhakimov restored the bugfix-197-treatEmptyStringAsNull branch April 10, 2020 14:02
@azatkhakimov azatkhakimov reopened this Apr 10, 2020
@devOpsHazelcast
Copy link
Contributor

Can one of the admins verify this patch?

Copy link

@leszko leszko left a comment

Choose a reason for hiding this comment

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

@jawitcher Thanks. I had a look at the PR and it looks like what we need.

Could you also do the following and then change it from DRAFT to the PR?

  1. Add the same logic for getMode()
  2. Add unit tests
  3. Format the code

Thanks!

@azatkhakimov azatkhakimov marked this pull request as ready for review April 14, 2020 13:42
@azatkhakimov
Copy link
Contributor Author

@jawitcher Thanks. I had a look at the PR and it looks like what we need.

Could you also do the following and then change it from DRAFT to the PR?

1. Add the same logic for `getMode()`

2. Add unit tests

3. Format the code

Thanks!

I revised the code in accordance with your notes

@leszko leszko requested a review from mesutcelik April 15, 2020 10:10
Copy link

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one minor comment. Other than that looks good to me.

@mesutcelik could you also look and approve?

Comment on lines 281 to 285
// given
Map<String, Comparable> properties = createProperties();
properties.put(SERVICE_NAME.key(), " ");
properties.put(SERVICE_DNS.key(), "service-dns");
new KubernetesConfig(properties);
Copy link

Choose a reason for hiding this comment

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

Suggested change
// given
Map<String, Comparable> properties = createProperties();
properties.put(SERVICE_NAME.key(), " ");
properties.put(SERVICE_DNS.key(), "service-dns");
new KubernetesConfig(properties);
// given
Map<String, Comparable> properties = createProperties();
properties.put(SERVICE_NAME.key(), " ");
properties.put(SERVICE_DNS.key(), "service-dns");
// when
KubernetesConfig config = new KubernetesConfig(properties);
// then
assertEquals("service-dns", config.getServiceDns());

Could you change this test to this format? And could you also change the other tests the same way?

Copy link
Contributor

@mesutcelik mesutcelik left a comment

Choose a reason for hiding this comment

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

👍 I think we can update helm chart readme how to use the workaround.. setting servine-name: empty_string to be able to use DNS mode. I will leave it to you @leszko

@mesutcelik
Copy link
Contributor

@jawitcher Can you please sign the CLA before the merge?

@azatkhakimov
Copy link
Contributor Author

@leszko , I changed it

@azatkhakimov
Copy link
Contributor Author

@jawitcher Can you please sign the CLA before the merge?

Hi. I signed it :)

@leszko
Copy link

leszko commented Apr 16, 2020

Looks great 💯

Thanks a lot for the change @jawitcher !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants