fix(kustomize): Updated generator support for both EnvSources and FileSources using [{key}=]{path} syntax #9900
Conversation
…nerators Added tests that validate support of env files and regular files aliased with keys when using kustomize during the render stage, and when running skaffold in dev mode.
…eSources using [{key}=]{path} syntax
Summary of ChangesHello @johanbcn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues within Skaffold's Kustomize integration, specifically concerning how Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where skaffold would fail to mirror or watch files from kustomize's configMapGenerator and secretGenerator when using env sources or the key=path syntax for files sources. The changes correctly parse these sources to identify the file paths. The accompanying test cases are well-written and cover the fixed scenarios.
My review focuses on improving code maintainability by addressing code duplication. I've suggested refactoring the duplicated logic for handling generator sources into helper functions in pkg/skaffold/render/renderer/kustomize/kustomize.go. This will make the code cleaner and easier to maintain.
| for _, generator := range content.ConfigMapGenerator { | ||
| deps = append(deps, sUtil.AbsolutePaths(dir, generator.FileSources)...) | ||
| envs := generator.EnvSources | ||
| var sources []string | ||
|
|
||
| if generator.FileSources != nil { | ||
| for _, f := range generator.FileSources { | ||
| // Entries from FileSources can take the form: [{key}=]{path} | ||
| i := strings.IndexRune(f, '=') | ||
| if i > -1 { | ||
| f = f[i+1:] | ||
| } | ||
| sources = append(sources, f) | ||
| } | ||
| } | ||
|
|
||
| sources = append(sources, generator.EnvSources...) | ||
| if generator.EnvSource != "" { | ||
| envs = append(envs, generator.EnvSource) | ||
| sources = append(sources, generator.EnvSource) | ||
| } | ||
| deps = append(deps, sUtil.AbsolutePaths(dir, envs)...) | ||
|
|
||
| deps = append(deps, sUtil.AbsolutePaths(dir, sources)...) | ||
| } | ||
|
|
||
| for _, generator := range content.SecretGenerator { | ||
| deps = append(deps, sUtil.AbsolutePaths(dir, generator.FileSources)...) | ||
| envs := generator.EnvSources | ||
| var sources []string | ||
|
|
||
| if generator.FileSources != nil { | ||
| for _, f := range generator.FileSources { | ||
| // Entries from FileSources can take the form: [{key}=]{path} | ||
| i := strings.IndexRune(f, '=') | ||
| if i > -1 { | ||
| f = f[i+1:] | ||
| } | ||
| sources = append(sources, f) | ||
| } | ||
| } | ||
|
|
||
| sources = append(sources, generator.EnvSources...) | ||
| if generator.EnvSource != "" { | ||
| envs = append(envs, generator.EnvSource) | ||
| sources = append(sources, generator.EnvSource) | ||
| } | ||
| deps = append(deps, sUtil.AbsolutePaths(dir, envs)...) | ||
|
|
||
| deps = append(deps, sUtil.AbsolutePaths(dir, sources)...) | ||
| } |
There was a problem hiding this comment.
The logic to collect dependency sources from ConfigMapGenerator (lines 550-570) is identical to the logic for SecretGenerator (lines 572-592). To avoid this duplication and improve maintainability, you can extract this logic into a helper function.
Here's an example of what that helper function could look like:
func getGeneratorSources(fileSources, envSources []string, envSource string) []string {
var sources []string
for _, f := range fileSources {
// Entries from FileSources can take the form: [{key}=]{path}
if _, path, found := strings.Cut(f, "="); found {
f = path
}
sources = append(sources, f)
}
sources = append(sources, envSources...)
if envSource != "" {
sources = append(sources, envSource)
}
return sources
}Using this helper function would simplify both loops in DependenciesForKustomization:
for _, generator := range content.ConfigMapGenerator {
sources := getGeneratorSources(generator.FileSources, generator.EnvSources, generator.EnvSource)
deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}
for _, generator := range content.SecretGenerator {
sources := getGeneratorSources(generator.FileSources, generator.EnvSources, generator.EnvSource)
deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}I also suggest using strings.Cut (available since Go 1.18) to make the path extraction from FileSources slightly cleaner.
1 participant
Fixes: #9338
Related: n/a
Merge before/after: n/a
Description
When using kustomize during the render stage, if files must be mirrored before rendering because they require modifications (when using --set flags, for example), files defined in the
envsection ofconfigMapGeneratorandsecretGeneratorare omitted, and thus the render ends up failing because it cannot find all the required filed.The error message in that case is similar to this:
Moreover, both on the previous situation and when using skaffold in dev mode (using the
skaffold devcommand), files defined in thefilessection of theconfigMapGeneratorandsecretGeneratorthat make use of the[{key}=]{path}syntax aren't properly recognized and the files neither won't be copied when mirroring or watched upon for changes in dev mode.The error message visible during the render stage is similar to this:
An example of the
[{key}=]{path}syntax can be seen in the second example from kustomize's reference documentation).This PR adds additional test cases for the situations described above and the required fixes.