Skip to content
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

Localize HelmChartInflationGenerator #5007

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion api/internal/localizer/builtinplugins.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ func (lbp *localizeBuiltinPlugins) Filter(plugins []*yaml.RNode) ([]*yaml.RNode,
Gvk: resid.Gvk{Version: konfig.BuiltinPluginApiVersion, Kind: builtinhelpers.SecretGenerator.String()},
Path: "envs",
},
types.FieldSpec{
Gvk: resid.Gvk{Version: konfig.BuiltinPluginApiVersion, Kind: builtinhelpers.HelmChartInflationGenerator.String()},
Path: "valuesFile",
},
types.FieldSpec{
Gvk: resid.Gvk{Version: konfig.BuiltinPluginApiVersion, Kind: builtinhelpers.PatchTransformer.String()},
Path: "path",
Expand Down Expand Up @@ -82,6 +86,24 @@ func (lbp *localizeBuiltinPlugins) Filter(plugins []*yaml.RNode) ([]*yaml.RNode,
return lbp.localizeAll(node)
},
},
yaml.FilterFunc(func(node *yaml.RNode) (*yaml.RNode, error) {
isHelm := node.GetApiVersion() == konfig.BuiltinPluginApiVersion &&
node.GetKind() == builtinhelpers.HelmChartInflationGenerator.String()
if !isHelm {
return node, nil
}
home, err := node.Pipe(yaml.Lookup("chartHome"))
if err != nil {
return nil, errors.Wrap(err)
}
if home == nil {
_, err = lbp.lc.copyChartHomeEntry("")
} else {
lbp.locPathFn = lbp.lc.copyChartHomeEntry
err = lbp.localizeScalar(home)
}
return node, errors.WrapPrefixf(err, "plugin %s", resid.FromRNode(node))
}),
fieldspec.Filter{
FieldSpec: types.FieldSpec{
Gvk: resid.Gvk{Version: konfig.BuiltinPluginApiVersion, Kind: builtinhelpers.PatchStrategicMergeTransformer.String()},
Expand All @@ -92,7 +114,6 @@ func (lbp *localizeBuiltinPlugins) Filter(plugins []*yaml.RNode) ([]*yaml.RNode,
return lbp.localizeAll(node)
},
})
// TODO(annasong): localize HelmChartInflationGenerator
if err != nil {
return nil, errors.Wrap(err)
}
Expand Down
119 changes: 113 additions & 6 deletions api/internal/localizer/localizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,8 +593,36 @@ patches:
}

func TestLocalizePluginsInlineAndFile(t *testing.T) {
kustAndPlugins := map[string]string{
"kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1
for _, test := range []struct {
name string
files map[string]string
}{
{
name: "generators",
files: map[string]string{
"kustomization.yaml": `generators:
- generator.yaml
- |
apiVersion: builtin
env: second.properties
kind: ConfigMapGenerator
metadata:
name: inline
`,
"generator.yaml": `apiVersion: builtin
env: first.properties
kind: ConfigMapGenerator
metadata:
name: file
`,
"first.properties": "APPLE=orange",
"second.properties": "BANANA=pear",
},
},
{
name: "transformers",
files: map[string]string{
"kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
transformers:
- |
Expand All @@ -605,16 +633,39 @@ transformers:
path: patchSM-one.yaml
- patch.yaml
`,
"patch.yaml": `apiVersion: builtin
"patch.yaml": `apiVersion: builtin
kind: PatchTransformer
metadata:
name: file
path: patchSM-two.yaml
`,
"patchSM-one.yaml": podConfiguration,
"patchSM-two.yaml": podConfiguration,
"patchSM-one.yaml": podConfiguration,
"patchSM-two.yaml": podConfiguration,
},
},
{
name: "validators",
files: map[string]string{
"kustomization.yaml": `validators:
- |
apiVersion: builtin
kind: ReplacementTransformer
metadata:
name: inline
replacements:
- path: first.yaml
- second.yaml
`,
"first.yaml": replacementTransformerWithPath,
"second.yaml": replacementTransformerWithPath,
"replacement.yaml": replacements,
},
},
} {
t.Run(test.name, func(t *testing.T) {
checkLocalizeInTargetSuccess(t, test.files)
})
}
checkLocalizeInTargetSuccess(t, kustAndPlugins)
}

func TestLocalizeMultiplePluginsInEntry(t *testing.T) {
Expand Down Expand Up @@ -1461,6 +1512,62 @@ func TestCopyChartHomeError(t *testing.T) {
}
}

func TestLocalizeGeneratorsHelm(t *testing.T) {
files := map[string]string{
"kustomization.yaml": `generators:
- default.yaml
Copy link
Contributor

@natasha41575 natasha41575 Jan 30, 2023

Choose a reason for hiding this comment

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

Does localize currently handle generators defined inline? This is not required for alpha imo, but if it already handled, can we add a test? If not, can we add a TODO?

Adding an example in case it's unclear what I'm referring to:

generators:
- |-
  apiVersion: builtin
  kind: HelmChartInflationGenerator
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Yes, we handle inline generators and I added a test.

- explicit.yaml
`,
"default.yaml": `apiVersion: builtin
kind: HelmChartInflationGenerator
metadata:
name: no-explicit-references
name: minecraft
releaseName: moria
repo: https://itzg.github.io/minecraft-server-charts
version: 3.1.3
`,
"explicit.yaml": `apiVersion: builtin
chartHome: home
kind: HelmChartInflationGenerator
metadata:
name: explicit-references
name: mapleStory
valuesFile: mapleValues.yaml
`,
"mapleValues.yaml": valuesFile,
"home/mapleStory/values.yaml": valuesFile,
"charts/minecraft/values.yaml": valuesFile,
}
checkLocalizeInTargetSuccess(t, files)
}

func TestLocalizeGeneratorsNoHelm(t *testing.T) {
files := map[string]string{
"kustomization.yaml": `generators:
- configMap.yaml
`,
"configMap.yaml": `apiVersion: builtin
kind: ConfigMapGenerator
literals:
- APPLE=orange
metadata:
name: not-helm-shouldn't-copy-default-helm-chart-home
`,
"charts/minecraft/values.yaml": valuesFile,
}
expected, actual := makeFileSystems(t, "/a", files)

err := Run("/a", "", "/dst", actual)
require.NoError(t, err)

addFiles(t, expected, "/dst", map[string]string{
"kustomization.yaml": files["kustomization.yaml"],
"configMap.yaml": files["configMap.yaml"],
})
checkFSys(t, expected, actual)
}

func TestLocalizeEmpty(t *testing.T) {
for name, kustomization := range map[string]string{
"file": `configurations:
Expand Down
4 changes: 1 addition & 3 deletions api/krusty/localizer/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,11 @@ func TestResourcesRepoNotFile(t *testing.T) {

err := localizer.Run(fsActual, testDir.String(), "", testDir.Join("dst"))

const readmeErr = `mapping values are not allowed in this context`
fileErr := fmt.Sprintf(`invalid resource at file "%s": MalformedYAMLError:`, repo)
fileErr := fmt.Sprintf(`invalid resource at file "%s": MalformedYAMLError`, repo)
rootErr := fmt.Sprintf(`unable to localize root "%s": unable to find one of 'kustomization.yaml', 'kustomization.yml' or 'Kustomization'`, repo)
var actualErr PathLocalizeError
require.ErrorAs(t, err, &actualErr)
require.Equal(t, repo, actualErr.Path)
require.ErrorContains(t, actualErr.FileError, readmeErr)
require.ErrorContains(t, actualErr.FileError, fileErr)
require.ErrorContains(t, actualErr.RootError, rootErr)
Comment on lines -455 to 461
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the readmeErr that's specific to the contents of the kustomize repo README.md and irrelevant to our test, which just expects that the README is invalid yaml.

The over-specificity of this test previously failed the ci pipeline on master: https://github.com/kubernetes-sigs/kustomize/actions/runs/4018832421/jobs/6904935339


Expand Down