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

[elasticsearch] Added health pod name override for compatibility #1058

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

tomhobson
Copy link
Contributor

@tomhobson tomhobson commented Feb 10, 2021

  • 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

Could not see the name overrides contained within the README.md, I can add them if needed.

This is to add support for tools like
https://github.com/roboll/helmfile and https://jenkins-x.io/v3/

As they generate a new pod each time, this creates a lot of noise and can cause merge conflicts, being able to override the health pod name prevents the pod being recreated when nothing has changed and enables smooth sailing.

@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 Feb 10, 2021

💚 CLA has been signed

@tomhobson tomhobson changed the title [elasticsearch] Added health pod name override for compatability [elasticsearch] Added health pod name override for compatibility Feb 11, 2021
@tomhobson tomhobson closed this Feb 11, 2021
@tomhobson tomhobson reopened this Feb 11, 2021
@jmlrt jmlrt added elasticsearch enhancement New feature or request labels Feb 24, 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
Copy link
Member

jmlrt commented Feb 24, 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.

While the code LGTM, could you add a bit more details about why this is needed?

"helm.sh/hook-delete-policy": hook-succeeded clean the test pod when test is successful.

Also "{{ .Release.Name }}-{{ randAlpha 5 | lower }}-test" should ensure that 2 different releases deployend in the same namespace should generate different pod names.

Could not see the name overrides contained within the README.md, I can add them if needed

nameOverride is described in https://github.com/elastic/helm-charts/blob/master/elasticsearch/README.md#L141, healthNameOverride should also have its description if this change is still needed.

@tomhobson
Copy link
Contributor Author

Yeah so, Helmfile generates all of the charts and values into one directory. This is then commited to a github repository for helm to run upgrades on.

With randomly generated names it means that every time a commit is done to the cluster repo, a new elasticsearch test pod is made. This is especially annoying when you have multiple commits at once, as it creates merge conflicts for the cluster PRs. Having to manually fix your CI/CD every hour is very annoying.

We've removed elasticsearch from our cluster for this reason

Allowing a health pod name override just means that if this pod is already created, it wont regenerate it and cause conflicts. If we add some new values to elasticsearch or upgrade the version, it will recreate.
I don't think it has any impact on functionality.

(Apologies for the Readme, fix incoming)

@tomhobson
Copy link
Contributor Author

@jmlrt sorry to be annoying, would like this to be merged before I have to do conflict fixing :)

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
Copy link
Member

jmlrt commented Jun 22, 2021

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 22, 2021

unrelated errors, merging

jmlrt added a commit that referenced this pull request Jul 6, 2021
jmlrt added a commit that referenced this pull request Jul 6, 2021
jmlrt added a commit that referenced this pull request Jul 6, 2021
jmlrt added a commit to nflaig/helm-charts that referenced this pull request Jul 6, 2021
@jmlrt jmlrt added the v6.8.17 label Jul 6, 2021
This was referenced Jul 8, 2021
@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.

3 participants