Add support for Options.HelmValuesOptions #146

aiyengar2 · 2021-03-27T22:34:40Z

This will allow consumers of this package to provide non-default Helm values.yamls to lint against.

Note:: First commit comes from #144, which will need to be merged in before this is merged. Alternatively, I can add this commit to that PR and drop this instead.

Related Issue: #145

aiyengar2 · 2021-03-27T22:37:32Z

pkg/lintcontext/parse_yaml.go

@@ -80,10 +82,9 @@ func (l *lintContextImpl) renderHelmChart(dir string) (map[string]string, error)
 	if err := chrt.Validate(); err != nil {
 		return nil, err
 	}
-	valOpts := &values.Options{ValueFiles: []string{filepath.Join(dir, "values.yaml")}}


This code isn't necessary since the Helm parser already uses the default values.yaml within the chart when it executes the render.

viswajithiii · 2021-03-29T16:55:06Z

pkg/lintcontext/parse_yaml.go

@@ -80,10 +82,9 @@ func (l *lintContextImpl) renderHelmChart(dir string) (map[string]string, error)
 	if err := chrt.Validate(); err != nil {
 		return nil, err
 	}
-	valOpts := &values.Options{ValueFiles: []string{filepath.Join(dir, "values.yaml")}}


viswajithiii · 2021-03-29T17:05:00Z

pkg/lintcontext/create_contexts.go

+
+	// HelmValuesOptions provide options for additional values.yamls that can be provided to Helm on loading a chart
+	// These will be ignored for contexts that are not Helm-based
+	HelmValuesOptions values.Options


So, one piece of complexity here is that CreateContexts can potentially create multiple Helm contexts by crawling directories, whereas HelmValuesOptions would be different for each Helm chart. What are your thoughts here? How are you using this function?

Ah you are right. I was only using CreateContexts to create a single context, not multiple.

IMO, we should have lintContextImpl continue to take in a single values.Options and update the exported lintcontext.Options to have HelmValuesOptions []values.Options.

Then, on CreateContexts, we assert that len(filesOrDirs) == len(options.HelmValuesOptions) or return an error.

Does this sound good to you? I can perform this change on rebasing this PR.

The problem is that CreateContexts is recursive, so len(filesOrDirs) == len(options.HelmValuesOptions) is not a sufficient condition. Plus, I prefer to avoid parallel arrays anyway.

Long term, in the context of someone running kube-linter lint, I would like to move to a model where the user stores these options as code, along with their Helm chart, and KubeLinter reads them and users them. (See #49.) I also want users to be able to specify multiple values.Options for a single Helm chart, too, because it would be useful for a user to lint their Helm chart against multiple different settings of values.

For library usage (which is the immediate use-case here), my suggestion is as follows: create a separate function that takes in one directory as input, which must contain a single Helm chart (if not, it returns an error), and then renders it. This function can take in a new Options argument, which includes an array of HelmValuesOptions, and also embeds the Options you created in #144 in it (so that people can use that to pass customDecoders). The function then returns a list of LintContexts, one for each HelmValuesOption passed in (or, if no HelmValuesOption was passed in, it uses the default values.yaml and returns exactly one LintContext). We can avoid duplication by calling this function inside the isHelm branch of CreateContexts. This seems like it will be compatible with the changes we want to make down the road.

Does this make sense to you?

The problem is that CreateContexts is recursive, so len(filesOrDirs) == len(options.HelmValuesOptions) is not a sufficient condition. Plus, I prefer to avoid parallel arrays anyway.

I see your point. I didn't realize that CreateContexts was recursive till you just mentioned it 😅 but that makes sense.

For library usage (which is the immediate use-case here), my suggestion is as follows: create a separate function that takes in one directory as input, which must contain a single Helm chart (if not, it returns an error), and then renders it.

Sounds good to me! This was how I was using it anyways, so this would be perfect for my solution.

Long term, in the context of someone running kube-linter lint, I would like to move to a model where the user stores these options as code, along with their Helm chart, and KubeLinter reads them and users them. (See #49.) I also want users to be able to specify multiple values.Options for a single Helm chart, too, because it would be useful for a user to lint their Helm chart against multiple different settings of values.

This is incredibly similar to what I've been working on FYI. If you are interested in taking a peek behind the curtain, here is my code:
https://github.com/aiyengar2/charts-testing/tree/0dade8345626cd045fa9359e9018e20bebe0899a/testing

TLDR of the design:

You define a testSuite, which replaces the Register code you use for registering templates / checks so that they can be dynamically provided by the user

You then parse templates into the testSuite, where you also have the ability to set a custom scheme if you are using CRDs in your templates

You then add a Test (e.g. testBuilder), specifying the templates it applies on (All|Include|Exclude), subsets of resources you want to parse within those templates (On|OnFilter|OnMatcher), and any function that satisfies the reflection-based requirements for a DoFunc

Finally, you call Run or RunWithContext (if you want to pass params to a DoFunc)

https://github.com/aiyengar2/charts-testing/tree/0dade8345626cd045fa9359e9018e20bebe0899a/tests/common contains some examples of DoFuncs that can be written using this framework (only involves writing a struct and then writing the function based on that struct)

https://github.com/aiyengar2/charts-testing/tree/0dade8345626cd045fa9359e9018e20bebe0899a/tests/template/tests contains example code that uses this framework. main_test.go and example_*_test.go might be interesting to you.

For repository-specific template configurations:

While parsing in templates, I make sure that it's only a single Helm chart

I add each Helm chart + values.yamls pair as a separate template here

w.r.t. the existing code in kube-linter, you could translate each template generically into a DoFunc based struct + function and convert all your built-in tests into Tests within a single testSuite.

I haven't quite figured out how to upstream it yet, but will do when I get a chance!

Also please don't hold me on the "public" facing API signatures just yet since it's not ready yet 😄 ... will need to move it into another repository before it's ready to be consumed.

Ooh this is pretty cool. Definitely looking forward to seeing where this ends up, and learning from it to figure out what makes sense for us to do in this repo (ie, to enable this functionality completely via options to the CLI).

viswajithiii

Looks good. Can you rebase on main? I can merge after that.

viswajithiii · 2021-05-07T21:54:10Z

Hi @aiyengar2, just wanted to check in on this. Will you be able to rebase and merge this one?

Allows users to provide options for creating lint context. Related Issue: stackrox#141 Signed-off-by: Arvind Iyengar <arvind.iyengar@rancher.com>

nilic · 2021-08-10T09:22:16Z

Any chance for merging this PR?

janisz · 2022-08-29T11:47:45Z

What's the status of this?

aiyengar2 commented Mar 27, 2021

View reviewed changes

aiyengar2 mentioned this pull request Mar 27, 2021

Add CreateContextWithOptions #144

Merged

aiyengar2 force-pushed the allow_helm_values_overrides branch from 58ffbd1 to 9ca1456 Compare March 28, 2021 01:02

viswajithiii reviewed Mar 29, 2021

View reviewed changes

aiyengar2 requested a review from viswajithiii March 29, 2021 17:30

viswajithiii approved these changes Mar 31, 2021

View reviewed changes

aiyengar2 force-pushed the allow_helm_values_overrides branch 2 times, most recently from c9c732c to e90231d Compare May 13, 2021 19:49

Add CreateContextWithOptions for lintcontext

53e3415

Allows users to provide options for creating lint context. Related Issue: stackrox#141 Signed-off-by: Arvind Iyengar <arvind.iyengar@rancher.com>

aiyengar2 force-pushed the allow_helm_values_overrides branch from e90231d to 53e3415 Compare May 13, 2021 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Options.HelmValuesOptions #146

Add support for Options.HelmValuesOptions #146

aiyengar2 commented Mar 27, 2021 •

edited

aiyengar2 Mar 27, 2021

viswajithiii Mar 29, 2021

viswajithiii Mar 29, 2021

viswajithiii Mar 29, 2021

aiyengar2 Mar 29, 2021 •

edited

viswajithiii Mar 29, 2021

aiyengar2 Mar 29, 2021

aiyengar2 Mar 29, 2021 •

edited

viswajithiii Mar 31, 2021

viswajithiii left a comment

viswajithiii commented May 7, 2021

nilic commented Aug 10, 2021

janisz commented Aug 29, 2022

Add support for Options.HelmValuesOptions #146

Are you sure you want to change the base?

Add support for Options.HelmValuesOptions #146

Conversation

aiyengar2 commented Mar 27, 2021 • edited

aiyengar2 Mar 27, 2021

Choose a reason for hiding this comment

viswajithiii Mar 29, 2021

Choose a reason for hiding this comment

viswajithiii Mar 29, 2021

Choose a reason for hiding this comment

viswajithiii Mar 29, 2021

Choose a reason for hiding this comment

aiyengar2 Mar 29, 2021 • edited

Choose a reason for hiding this comment

viswajithiii Mar 29, 2021

Choose a reason for hiding this comment

aiyengar2 Mar 29, 2021

Choose a reason for hiding this comment

aiyengar2 Mar 29, 2021 • edited

Choose a reason for hiding this comment

viswajithiii Mar 31, 2021

Choose a reason for hiding this comment

viswajithiii left a comment

Choose a reason for hiding this comment

viswajithiii commented May 7, 2021

nilic commented Aug 10, 2021

janisz commented Aug 29, 2022

aiyengar2 commented Mar 27, 2021 •

edited

aiyengar2 Mar 29, 2021 •

edited

aiyengar2 Mar 29, 2021 •

edited