Document migration path to ingress-nginx chart from stable/nginx-ingress #6038
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
…inx-ingress Signed-off-by: Scott Rigby <[email protected]>
scottrigby
force-pushed
the
migration-from-stable-nginx-ingress
branch
from
8192f35
to
f78693f
Compare
Signed-off-by: Scott Rigby <[email protected]>
|
/assign @ChiefAlexander |
|
|
||
| The following table lists the configurable parameters of the ingress-nginx chart and their default values. |
There was a problem hiding this comment.
just remove this section without a replacement?
There was a problem hiding this comment.
Yeah, why was this removed?
There was a problem hiding this comment.
Hey good question – I should have given more context, have been doing this a lot lately trying to prep the move from stable for charts that have been in discussion for a while.
These chart README templates have been copied and re-copied across charts for years, and now that we're moving repos and counting down the days to Helm 2's end of support, it seems like a good time to simplify things.
For this issue, I needed to add an Upgrading section, for the remaining item in #5161 before we can mark the stable/nginx-ingress chart deprecated and point users here. I took the opportunity to make a few other section cleanup changes. Major differences are:
- Added Helm 2 and Helm 3 commands, with links to Helm 3 command reference links (from there, users can select Helm 2 from the version dropdown if they wish)
- Removed the options table that's duplicated from (often out of sync with)
values.yamlin favor ofhelm inspect(Helm 2) andhelm show values(Helm 3). Thevalues.yamlfile is well organized and commented, giving users a much better (and never out of sync) reference to what they can configure.
The rest of the changes I made were capitalization, removing some other duplicated instructions, linking to the better docs now at helm.sh/docs (they were not always this complete in the past when these README templates were initially written), nesting sub-headers (the configuration-related H2s now H3s underneath the "configure" H2, etc).
Related discussion at prometheus-community/helm-charts#14, where we're moving all the prometheus-related charts.
Personally I think it's cleaner this way, but lmk what you think 🙂
There was a problem hiding this comment.
This is great. I had no idea about the helm show values. I see now that is commented down later.
There was a problem hiding this comment.
Removed the options table that's duplicated from (often out of sync with) values.yaml in favor of helm inspect (Helm 2) and helm show values (Helm 3).
Makes sense. +1 from me
|
|
||
| Alternatively, a YAML file that specifies the values for the parameters can be provided while installing the chart. For example, | ||
| ### Migrating from stable/nginx-ingress |
There was a problem hiding this comment.
Just making sure, does thumbs up mean you think this section is good enough to deprecate the stable/nginx-ingress chart? I think the sooner there aren't two locations for users the better. We can still work to improve this, but just checking how you feel for this stage of the plan to officially deprecate stable and move here?
There was a problem hiding this comment.
This section looks reasonable and clear about what to expect
|
|
||
| ```console | ||
| $ helm install ingress-nginx --set controller.extraArgs.v=2 | ||
| # Helm 2 |
There was a problem hiding this comment.
@ChiefAlexander makes sense to keep mentioning helm2?
There was a problem hiding this comment.
Technically this chart should support either in its current form. I don't really mind leaving the references until helm 2 is fully dedicated. That or we flip the API version in the chart.yaml to the helm 3 version and just force helm 3 🤷♂️
There was a problem hiding this comment.
Helm 2 is still supported until Nov 13th, at which point I was thinking we should just remove those references from the README and docs.
@ChiefAlexander I would love to see moving to chart api v2, but would recommend keeping this v1 chart intact until after the Helm 2 support end date. One fun option could be to start on a v2 chart alongside this one (we just can't have two charts by the same name in the same helm repo index, even if they're two different chart api versions. So it could be named differently until then, or just not added to the repo index until then. Or we could just wait). One really nice feature as of Helm v3.1.0 is the post-renderer flag, which I hope will encourage chart api v2 charts to be dramatically simpler (less if/else logic for every use case within gotemplating) because end users can add their edge cases with a post renderer, so no need to add every use case in the world to ever chart after that. Would love to chat more about this one when you all have time! Thinking it's out of scope for this PR though
|
@scottrigby thank you for working on this 🎉 |
|
|
||
| The following table lists the configurable parameters of the ingress-nginx chart and their default values. |
There was a problem hiding this comment.
Yeah, why was this removed?
|
|
||
| ```console | ||
| $ helm install ingress-nginx --set controller.extraArgs.v=2 | ||
| # Helm 2 |
There was a problem hiding this comment.
Technically this chart should support either in its current form. I don't really mind leaving the references until helm 2 is fully dedicated. That or we flip the API version in the chart.yaml to the helm 3 version and just force helm 3 🤷♂️
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/approve |
|
Oh, wait. We should also bump the chart version before merging this in. /approve cancel |
Signed-off-by: Scott Rigby <[email protected]>
|
Damnit lol. Bumped chart PATCH version 👍 |
|
/lgtm |
|
@scottrigby thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ChiefAlexander, scottrigby The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Last step for #5161
See https://github.com/helm/charts/issues/21103