opensearch-project / helm-charts

:wheel_of_dharma: A community repository for Helm Charts of OpenSearch Project.
https://opensearch.org/docs/latest/opensearch/install/helm/
Apache License 2.0
173 stars 234 forks source link

Customize opensearch deployment annotation through values.yaml #407

Closed prathaptce closed 1 year ago

prathaptce commented 1 year ago

Description

[Currently, OpenSearch deployment is not customizable using values.yaml . I'm trying to add deployment annotation in the values.yaml , which will be used by the deployment.yaml and configures the metadata annotation of the deployment.]

Issues Resolved

[https://github.com/opensearch-project/opensearch-devops/issues/115]

Check List

For any changes to files within Helm chart directories:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

prathaptce commented 1 year ago

@bbarani , Hi, I'm new to this review process. I have submitted the changes needed for customizing the OpenSearch deployment. Can someone review these changes? thanks

prudhvigodithi commented 1 year ago

Hey @prathaptce thanks for the contribution, please check the following points. 1) The DCO check is missing, please make sure the commits are signed. 2) The Helm chart version has to be bumped (sample PR). 3) The CHANGELOG.md has to be recorded along with if required updating README.md.

Apart from this I will review the technical aspects of the chart shortly, thank you.

prathaptce commented 1 year ago

Hey @prathaptce thanks for the contribution, please check the following points.

  1. The DCO check is missing, please make sure the commits are signed.
  2. The Helm chart version has to be bumped (sample PR).
  3. The CHANGELOG.md has to be recorded along with if required updating README.md.

Apart from this I will review the technical aspects of the chart shortly, thank you.

Now, DCO check is passing, helm chart version is bumped & 'CHANGELOG.md' is updated with the changes done for this PR

prudhvigodithi commented 1 year ago

Hey @prathaptce apart from my previous suggestions, can you take care of the lint errors also please update the README files in the chart with the new annotation changes. Thank you

prathaptce commented 1 year ago

Hey @prathaptce apart from my previous suggestions, can you take care of the lint errors also please update the README files in the chart with the new annotation changes. Thank you

I'm not able to execute ct lint locally, is there a way I can trigger the ct lint job in GitHub & check the error? I'm not sure which line is creating the lint error

prathaptce commented 1 year ago

prudhvigodithi

@prudhvigodithi, I have addressed all the comments. Let me know if the lint error still exists. thanks

prudhvigodithi commented 1 year ago

Thanks @prathaptce , @TheAlgo and @peterzhuamazon can you please review ? Thank you

prathaptce commented 1 year ago

Thanks @prathaptce , @TheAlgo and @peterzhuamazon can you please review ? Thank you

@TheAlgo @peterzhuamazon, can you please review? thank you

prudhvigodithi commented 1 year ago

Hey @prathaptce I did notice one thing the Chary.yaml file is not incremented, sample PR https://github.com/opensearch-project/helm-charts/pull/375/files, can you please check.

prathaptce commented 1 year ago

Hey @prathaptce I did notice one thing the Chary.yaml file is not incremented, sample PR https://github.com/opensearch-project/helm-charts/pull/375/files, can you please check.

Just now pumped up the version numbers in Chart.yaml. Please check now

prudhvigodithi commented 1 year ago

Pinging @TheAlgo and @peterzhuamazon :)

prathaptce commented 1 year ago

hi @TheAlgo @peterzhuamazon, Can you please review the changes & let me know your comments?

prathaptce commented 1 year ago

Can this PR merged? if no other comments. thanks

prudhvigodithi commented 1 year ago

Hey @TheAlgo @peterzhuamazon please add your approval. Thanks for your patience @prathaptce Thank you

TheAlgo commented 1 year ago

Hey @TheAlgo @peterzhuamazon please add your approval. Thanks for your patience @prathaptce Thank you

Will check this week. Apologies for delays.

prathaptce commented 1 year ago

@TheAlgo .Thank you. Can you please merge this PR?

bbarani commented 1 year ago

Merging the PR as its approved.

prudhvigodithi commented 1 year ago

Hey @prathaptce can you please backport to 1.x? as I see this feature is useful for 1.x version as well. Thank you

prathaptce commented 1 year ago

Sure @prudhvigodithi, I will raise the PR for 1.x as well