Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

empty properties treated as null #201

Merged
merged 3 commits into from
Apr 16, 2020
Merged

empty properties treated as null #201

merged 3 commits into from
Apr 16, 2020

Conversation

azatkhakimov
Copy link
Contributor

@azatkhakimov azatkhakimov commented Apr 10, 2020 •
edited
Loading

No description provided.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Apr 10, 2020 •
edited
Loading

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
@azatkhakimov azatkhakimov mentioned this pull request Apr 10, 2020
Closed
@devOpsHazelcast
Copy link
Contributor

Can one of the admins verify this patch?

leszko
leszko reviewed Apr 14, 2020
View reviewed changes
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
leszko
leszko approved these changes Apr 15, 2020
View reviewed changes
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?

src/test/java/com/hazelcast/kubernetes/KubernetesConfigTest.java Outdated
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?

mesutcelik
mesutcelik approved these changes Apr 15, 2020
View reviewed changes
Copy link

@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

@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 !

@leszko leszko merged commit 17a9570 into hazelcast:master Apr 16, 2020
@hasancelik hasancelik mentioned this pull request Jul 13, 2020
Merged
@leszko leszko added this to the 2.0.2 milestone Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mesutcelik mesutcelik mesutcelik approved these changes

@leszko leszko leszko approved these changes

Assignees
No one assigned
Labels
Source: Community
Projects
None yet
Milestone
2.0.2
Development

Successfully merging this pull request may close these issues.
5 participants
@azatkhakimov @devOpsHazelcast @mesutcelik @leszko @Holmistr