Updated default values.yaml with available options #387
Conversation
krishnakv
requested review from
marcofranssen,
kfox1111,
developer-guy,
dfeldman,
faisal-memon,
mrsabath and
edwbuck
as code owners
|
Thanks @krishnakv for submitting this. |
charts/spire/values.yaml
Outdated
| # -- Set the jwt issuer | ||
| jwtIssuer: oidc-discovery.example.org | ||
| # -- Override all instances of bundleConfigMap | ||
| ## @param global.spire.bundleConfigMap A configmap containing the Spire bundle |
There was a problem hiding this comment.
Im not familiar with the syntax youre using. The @param and @section markers. what do they do?
There was a problem hiding this comment.
Hi @faisal-memon , its just the convention used by bitnami charts and I don't think it offers any specific benefits beyond tagging the different sections. Looking through the helm reference, they are suggesting just starting the comment with the variable name - https://helm.sh/docs/chart_best_practices/values/ . I am happy to change if this format looks better. I will be fixing the linting errors (training spaces) and seeing if the docs need any updates over the next couple of days and sending an updated version.
There was a problem hiding this comment.
Thanks for working on this! :)
Currently all the doc strings are formatted for reading by https://github.com/norwoodj/helm-docs and its generating the README.md automatically. Its why the tests are broken on the PR. So the structure of the comments is important unless we change the tool being used to generate the docs. If you know a better one then the helm-docs one, that would be good to discuss. We've hit some limitations with helm-docs. Otherwise, we probably want to stick to the existing formatting structure so it works with the existing tooling.
There was a problem hiding this comment.
Lets go with this, will dig into the format and rework.
There was a problem hiding this comment.
How are we going to keep all of this in sync with the source of thruth?
Once we break out charts this is going to be problematic to even version the charts. Each individual chart holds the values. This composite chart just combines those defined by the dependency charts.
IMHO best we can do is adding following to the readme as I already expressed in the sync up meeting.
# To see all configurable options please see https://github.com/spiffe/helm-charts/blob/main/charts/spire/README.md#values
If we want to show the values of dependency charts using helm cli we should probably contribute that implementation of rendering the values of dependency charts there.
This is going to be a maintenance burden to keep in sync. It feels like a workaround for a shortcoming in the helm cli.
|
@marcofranssen as I understand it, the concern expressed is there are tons of options, and its hard to know which ones you need as a regular user. Just pointing them at a fully documented README with all the options in it wouldn't solve the problem. The idea was to put a limited set of values in charts/spire and generate README from it so that if a regular user, needing nothing special were to pick up the spire chart, they could look at the README or values file and figure out the few options they are likely to need to set. if anything more was needed, the README can reference the other chart saying "If you need advanced/unusual configuration options, please see the individual charts README's for more options. This would make onboarding significantly easier, and solve a problem seen out in the field. |
|
I think we are onto something here. Probably figure out what bitnami is doing in their CI. They are able to group those values using the annotations as proposed here by @krishnakv. Doing so we don't have to duplicate the values, but can point users at a more structured readme. So maybe that is the way forward. I checked this chart. https://artifacthub.io/packages/helm/bitnami/contour Their readme is grouped by the |
|
Yeah. I like their rendered readme's. That looks like a nice improvement. There a way we can surface the most common options though? Maybe some annotation? That still was a concern raised and documenting all options still doesn't solve that use case. |
|
Went a bit into the rabbit hole and here is a comparison of the approaches used by us v/s bitnami.
Happy I got to dig into this today. Might solve the issue around making changes in multiple places. The question, though, is - which tool do we go with? Is having named sections important enough for us that we switch tools? In both cases, we can generate the data in README from the values file. Can rework the chart accordingly. |
|
I think the named section thing would be a great way to flag options as "commonly used" and "uncommon" or something so that we can float the few options that most users will want to look at, up to the top. It would be way less confusing to most users then to find the options they need to override, while allowing other users to still be able to override all the things they may need to. |
|
Third possibility is to add this group by feature to helm-docs Maybe doing a poc of both your suggestions can help us make a more informed decision as well seeing the end result and consequences of either approach. |
|
Hi Folks, attached readme files generated by |
|
Nice. Thanks for doing the comparison. I like the readme-generator one better. |
|
Hi @edwbuck , based on your comment #307 (comment), have repointed this PR to Issue#406. |
Updated the default values.yaml with available options with minimal examples and help text where required. This follows the best practices of production helm charts such as bitnami postgresql and postgresql-ha charts.
This PR will allow users of Spire chart to see the values supported by the chart and craft their own versions using
helm show values <CHARTNAME>. Values will need to be updated whenever the public interface of the chart changes.Note: based on the comment #307 (comment), have modified this PR to align to Issue#406 below instead of Issue#307.
Resolves issue #406