Localize helm fields #4978
Localize helm fields #4978
Conversation
|
/cc @natasha41575 |
api/internal/localizer/localizer.go
Outdated
| kust.HelmGlobals.ChartHome = locDir | ||
| } | ||
| } | ||
| if useDefaultChartHome(kust) { |
There was a problem hiding this comment.
@natasha41575 As requested, I only localize the default chart home if a chart is present without a home.
| return errors.Wrap(err) | ||
| } | ||
| pathInDst := filepath.Join(dst, pathToCreate) | ||
| if info.Mode()&os.ModeSymlink == os.ModeSymlink { |
There was a problem hiding this comment.
@natasha41575 As requested, I don't follow symlinks.
As discussed before, our other option is to follow all symlinks that don't leave scope and write the file contents to the nominal (uncleaned) file paths in the chart home directory.
api/internal/localizer/localizer.go
Outdated
| return "", nil | ||
| } | ||
| return lc.localizeFile(entry) | ||
| } |
There was a problem hiding this comment.
super nit: can we merge this function into localizeFile.
There was a problem hiding this comment.
Agree that folding it in is cleaner. If you want to ignore errors for empty entry in some cases but not others, you could add an additional parameter to localizeFile, like localizeFile(path string, errIfEmpty bool).
There was a problem hiding this comment.
Done. I added the check for the empty string to localizeFile.
Thanks for the suggestion, @natasha41575. I considered this, but want to keep the method signature of localizeFile as func(string) (string, error) for ease of use localizeBuiltinPlugins' locPathFn func(string) (string, error) in builtinplugins.go file. I also reasoned that localize doesn't have to throw errors on empty entries since kustomize build will take care of this and we don't have to re-implement build functionality.
However, to keep treatment of empty fields consistent, I added the check for empty paths to localizeRoot: https://github.com/kubernetes-sigs/kustomize/pull/4978/files#diff-25faa786bf35469d391d5eef99ed1323623023442b177c1ed3e07f3e6709dac6R385-R387
I also added the following test to demonstrate localize's consistent treatment of empty paths: https://github.com/kubernetes-sigs/kustomize/pull/4978/files#diff-c21f7b3d0afda4a64163411e90e72d2fed6c3df14c7b6f8ed07237bb5e1fc91cR1317
|
/lgtm |
api/internal/localizer/localizer.go
Outdated
| return "", nil | ||
| } | ||
| return lc.localizeFile(entry) | ||
| } |
There was a problem hiding this comment.
Agree that folding it in is cleaner. If you want to ignore errors for empty entry in some cases but not others, you could add an additional parameter to localizeFile, like localizeFile(path string, errIfEmpty bool).
12d7075 to
314ca44
Compare
|
@annasong20: This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/label tide/merge-method-squash |
Split copyChartHome and use existence to prevent repeated copying.
a127010 to
a0109aa
Compare
There was a problem hiding this comment.
/lgtm
/approve
/hold
I'll try to update #4926 today or early tomorrow, all it should do is add a new additionalValuesFiles field to helmCharts.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: annasong20, natasha41575, yuwenma 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 |
|
/unhold |
This PR localizes the
valuesFileandchartHomefields of the helm fieldshelmChartInflationGenerator,helmCharts, andhelmGlobals.chartHomeis copied to the localize destination, but symlinks inside said directory are not followed.charts.