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

Add Helm Chart to install NKG #840

Merged
merged 13 commits into from
Jul 24, 2023
Merged

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Jul 10, 2023

Proposed changes

Problem:

As a potential user of NKG
I want to use a Helm chart to install the NGINX Kubernetes Gateway
So that I can use one command to install the NGINX Kubernetes Gateway
And so that I can easily update.

Solution:

  • Add a Helm chart to install the NGINX Kubernetes Gateway.
  • Create a static yaml for njs-module, a make command to generate the file, and add check in pipeline that the njs-modules.yaml file is up-to-date.
  • Add make command to lint the helm chart.
  • Add deployment instructions.
  • Add/ amend developer docs for Helm and for generators
  • Add stages in the CI pipeline to lint, test (install only), and publish charts to GitHub charts for edge and tags.

Testing: Local testing - verified local basic installation and modifications to the default values

Closes #614

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 10, 2023
@sjberman
Copy link
Contributor

We still need to refine it, but being that this work and #798 are slated for 0.6.0, I wonder if we get the latter done first so that we don't need the njs configmap in our helm chart.

Makefile Outdated Show resolved Hide resolved
deploy/helm-chart/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/templates/rbac.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/templates/rbac.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Shall we also update the main README?

deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/templates/_helpers.tpl Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ciarams87
Copy link
Member Author

ciarams87 commented Jul 11, 2023

We still need to refine it, but being that this work and #798 are slated for 0.6.0, I wonder if we get the latter done first so that we don't need the njs configmap in our helm chart.

@sjberman I think the work can coexist for now - if #798 gets done first, we can simplify the Helm chart as part of this work. If not, we can do it as part of that one.

@ciarams87 ciarams87 force-pushed the feat/helm-charts branch 2 times, most recently from c561ee0 to 27d7ecf Compare July 11, 2023 12:44
@sjberman
Copy link
Contributor

We still need to refine it, but being that this work and #798 are slated for 0.6.0, I wonder if we get the latter done first so that we don't need the njs configmap in our helm chart.

@sjberman I think the work can coexist for now - if #798 gets done first, we can simplify the Helm chart as part of this work. If not, we can do it as part of that one. We'll be removing the nginx-conf ConfigMap too anyway. There's a bit left to do here anyway 😄

The main reason I bring it up is that if we get helm charts released into 0.5, then we break the chart in 0.6 when we change containers up. Not a huge deal since we're still pre-v1, but avoiding breaking changes is nice.

@ciarams87 ciarams87 force-pushed the feat/helm-charts branch 4 times, most recently from af55b00 to e4ba68a Compare July 11, 2023 16:42
Makefile Outdated Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the feat/helm-charts branch 2 times, most recently from 9478919 to 92e048d Compare July 12, 2023 19:15
@ciarams87 ciarams87 marked this pull request as ready for review July 12, 2023 20:59
@ciarams87 ciarams87 requested a review from a team as a code owner July 12, 2023 20:59
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

could you update the PR description to follow problem/solution template and make sure there is a commit with problem/statement? (according to our guidelines)

Makefile Outdated Show resolved Hide resolved
deploy/helm-chart/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/Chart.yaml Show resolved Hide resolved
deploy/helm-chart/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/README.md Outdated Show resolved Hide resolved
deploy/helm-chart/templates/service.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
deploy/helm-chart/values.yaml Outdated Show resolved Hide resolved
@ciarams87 ciarams87 changed the title Initial helm chart Add Helm Chart to install NKG Jul 14, 2023
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

🚀

deploy/helm-chart/templates/gatewayclass.yaml Outdated Show resolved Hide resolved
docs/release-process.md Show resolved Hide resolved
@ciarams87 ciarams87 force-pushed the feat/helm-charts branch 4 times, most recently from 16db9c9 to 0825933 Compare July 24, 2023 13:26
@ciarams87 ciarams87 merged commit 5547fe5 into nginxinc:main Jul 24, 2023
20 checks passed
@ciarams87 ciarams87 deleted the feat/helm-charts branch July 24, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Helm Charts
6 participants