Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

Conversation

@DavidHuie
Copy link
Contributor

Many NGINX configuration parameters cannot be overridden with the
nginx-ingress Helm chart. With this change, a custom nginx.tmpl
template can be provided, which allows for any configuration value to
be changed.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 26, 2017
@DavidHuie
Copy link
Contributor Author

CLA's ready.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 26, 2017
@s4nch3z
Copy link
Contributor

s4nch3z commented Sep 27, 2017

It looks like we have a duplicate implementation. You were 8 hours ahead of me =). However, mine includes DHParam stuff
#2342

@viglesiasce
Copy link
Contributor

I like this implementation of the custom config better because it allows for an arbitrary configmap name and key.

@s4nch3z can you submit just the DHparam change in your PR?

@viglesiasce
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 1, 2017
@killfill
Copy link

killfill commented Oct 3, 2017

+1 !

@killfill
Copy link

killfill commented Oct 3, 2017

BTW, the daemonset file seems to be missing the change to do something with the customTemplate variables.

Copying the change from the deployment file works fine.

Thanks!

@DavidHuie DavidHuie force-pushed the nginx-ingress-custom-templates branch from 311c893 to 8e72115 Compare October 3, 2017 19:03
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2017
@DavidHuie
Copy link
Contributor Author

@killfill just added! Sorry about that.

@mgoodness mgoodness self-assigned this Oct 12, 2017
@DavidHuie
Copy link
Contributor Author

@mgoodness any updates on this guy? Thanks.

@DavidHuie DavidHuie force-pushed the nginx-ingress-custom-templates branch 2 times, most recently from 4bc07d1 to c5d4098 Compare October 31, 2017 19:28
@DavidHuie DavidHuie force-pushed the nginx-ingress-custom-templates branch from c5d4098 to 7d42bfa Compare November 15, 2017 23:29
@DavidHuie
Copy link
Contributor Author

@mgoodness @viglesiasce Can I do anything to move this forward? Thanks.

@jeff-french
Copy link
Contributor

+1 for merging this. Looks like it's been ready for over a month. Any way to get this merged?

@jeff-french
Copy link
Contributor

Well, it looks like the chart version will need to be bumped again since PR #2573 took 0.8.12

Many NGINX configuration parameters cannot be overridden with the
nginx-ingress Helm chart. With this change, a custom nginx.tmpl
template can be provided, which allows for any configuration value to
be changed.
@DavidHuie DavidHuie force-pushed the nginx-ingress-custom-templates branch from 7d42bfa to 7b8ab30 Compare November 18, 2017 00:31
@DavidHuie
Copy link
Contributor Author

@jeff-french fixed!

@jeff-french
Copy link
Contributor

@mgoodness @viglesiasce Is there anything else that needs to be done to get this merged?

@viglesiasce viglesiasce merged commit 20c6c76 into helm:master Nov 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants