Skip to content

Add Helm Charts #149

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

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Add Helm Charts #149

merged 1 commit into from
Dec 7, 2023

Conversation

osokin
Copy link
Contributor

@osokin osokin commented Nov 29, 2023

Proposed changes

Add Helm Charts

Checklist

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

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md and CHANGELOG.md)

Copy link
Collaborator

@ciroque ciroque left a comment

Choose a reason for hiding this comment

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

Hi @osokin , really appreciate the contribution! This is one that has been hanging out there for a while; knew it had to be done, but didn't find the motivation to actually do it.

This looks really good, I left a couple of comments about referring to support options as that has been somewhat of a sticky proposition during development. Mostly need to know if @brianehlert is okay using that email address, or if there is another he'd prefer.

The only other thing is around a recent upgrade to allow TLS usage. As detailed in the file comments, I am okay with moving forward with merging this PR as long as we follow up pretty quickly with adding the new TLS fields to the ConfigMap. I'll leave the decision to you. If you choose to kick that can down the road I would ask that you create a new Issue so the work doesn't fall by the wayside.

Again, thanks so much for the contribution, this is really helpful!

@osokin osokin marked this pull request as draft November 29, 2023 22:22
@osokin
Copy link
Contributor Author

osokin commented Nov 29, 2023

Hi @ciroque, thanks for the review! I've just updated the pull-request based on your suggestions and recommendations.

@osokin osokin marked this pull request as ready for review November 29, 2023 22:51
@ciroque ciroque merged commit 360536b into nginxinc:main Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants