Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

[elasticsearch] add value to disable service #1115

Merged
merged 1 commit into from Jul 6, 2021

Conversation

nflaig
Copy link
Contributor

@nflaig nflaig commented Mar 18, 2021

Hey guys,

right now it is not possible to completely disable the service but it is not required if the helm chart is deployed to a cluster with a service mesh (consul, istio, etc.).

Since the service.enabled does not apply to the headless service a different name could be considered but I think the issue rather is that both services use the same service value rather than service and serviceHeadless.

Fix #998

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Mar 18, 2021

💚 CLA has been signed

jmlrt
jmlrt previously approved these changes Apr 12, 2021
Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

LGTM⛴

@jmlrt jmlrt added elasticsearch enhancement New feature or request labels Apr 12, 2021
@jmlrt
Copy link
Member

jmlrt commented Apr 12, 2021

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Apr 12, 2021

will need to be rebased after #1141 merge to fix tests

@nflaig nflaig force-pushed the feature/disable_service branch 2 times, most recently from 4a0a0c0 to 210671e Compare April 12, 2021 21:14
@nflaig
Copy link
Contributor Author

nflaig commented Apr 12, 2021

@jmlrt Thanks for the review! I added the new value to the README and will rebase the branch if required

@jmlrt
Copy link
Member

jmlrt commented Jul 6, 2021

jenkins test this please

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Hi @nflaig, I rebased your branch on master and fixed the conflicts.
The change LGTM to me.

Adding a test in https://github.com/elastic/helm-charts/blob/master/elasticsearch/tests/elasticsearch_test.py to ensure that the service isn't created when setting service.enabled=false would be great.

Are you interested in adding this test? Otherwise I think we can merge if tests are successful.

@nflaig
Copy link
Contributor Author

nflaig commented Jul 6, 2021

@jmlrt Thanks for rebasing the branch. I added a test to check if disabling the service works as expected if service.enabled is set to false. Also I noticed that the commit message was somehow overwritten I updated the commit to have the original message, I hope thats fine

Signed-off-by: nflaig <nflaig@protonmail.com>
@jmlrt
Copy link
Member

jmlrt commented Jul 6, 2021

Thanks for rebasing the branch. I added a test to check if disabling the service works as expected if service.enabled is set to false.

👍🏻

Also I noticed that the commit message was somehow overwritten I updated the commit to have the original message, I hope thats fine

Yes, sorry I messed it a bit while rebasing...

@jmlrt
Copy link
Member

jmlrt commented Jul 6, 2021

jenkins test this please

@jmlrt jmlrt merged commit 0e5a168 into elastic:master Jul 6, 2021
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Sep 13, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Sep 13, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Sep 13, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>
jmlrt added a commit that referenced this pull request Sep 21, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
jmlrt added a commit that referenced this pull request Sep 21, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
@jmlrt jmlrt added the v7.14.2 label Sep 21, 2021
jmlrt added a commit that referenced this pull request Sep 21, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
jmlrt pushed a commit to jmlrt/helm-charts that referenced this pull request Sep 22, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>
jmlrt added a commit that referenced this pull request Sep 22, 2021
Signed-off-by: nflaig <nflaig@protonmail.com>

Co-authored-by: Nico Flaig <nflaig@protonmail.com>
@jmlrt jmlrt mentioned this pull request Mar 8, 2022
@jmlrt jmlrt mentioned this pull request Apr 21, 2022
This was referenced Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[elasticsearch] Add "service.enabled" parameter
3 participants