chore: cherrypick 3 fixes onto the release/v2.14 branch#9705
Conversation
plumpy
commented
Feb 4, 2025
- fix: (helm) Add expand env template for dependsOn, fix concurrent installation (fix(helm): Add expand env template for dependsOn, fix concurrent installation #9689)
- feat: revert "feat: transform imagePullPolicy when using local cluster (feat: transform imagePullPolicy when using local cluster #9495)" (feat: revert "feat: transform imagePullPolicy when using local cluster (#9495)" #9703)
- fix(helm): Fix helm package installation order (fix(helm): Fix helm package installation order #9693)
…tallation (GoogleContainerTools#9689) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> --------- Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
…9693) * fix(helm): Fix helm package installation order * fix copyright Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> * tests Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> * lint Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> * fixes Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> * fix linters Signed-off-by: Suleiman Dibirov <idsulik@gmail.com> --------- Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
ghost
left a comment
There was a problem hiding this comment.
Hi @plumpy, reviewers, and future viewers!
I'm Gemini, and I'm currently reviewing this pull request. In the meantime, I'm providing a summary for you and other reviewers to quickly get up to speed on the changes and intent.
This pull request, titled "cherrypicks", is a collection of three cherry-picked commits:
- fix: (helm) Add expand env template for dependsOn, fix concurrent installation (#9689): This commit addresses issues with concurrent Helm installations and environment variable expansion within
dependsOnspecifications. It appears to involve significant changes topkg/skaffold/deploy/helm/dependencygraph.go(lines 1-220) andpkg/skaffold/deploy/helm/dependencygraph_test.go(lines 1-320), adding robust dependency graph management and cycle detection. The changes inpkg/skaffold/deploy/helm/helm.go(lines 261-343) reflect the improved dependency handling, ensuring releases are deployed in the correct order, even with concurrency. - feat: revert "feat: transform imagePullPolicy when using local cluster (#9495)" (#9703): This commit reverts a previous feature that modified the
imagePullPolicywhen using a local cluster. This involved changes topkg/skaffold/deploy/kubectl/kubectl.go(lines 241-253) and the removal ofpkg/skaffold/kubernetes/manifest/image_pull_policy.go(lines 1-88) andpkg/skaffold/kubernetes/manifest/image_pull_policy_test.go(lines 1-67), suggesting a decision to remove this feature altogether. - fix(helm): Fix helm package installation order (#9693): This commit focuses on correcting the installation order of Helm packages. It includes modifications to
pkg/skaffold/deploy/helm/helm.go(lines 336-343), specifically handling the expansion of release names using environment templates.
I'll provide a more detailed review shortly. In the meantime, please let me know if you have any questions.
And now, a little poem to lighten the mood:
Code flows like a river,
Cherry-picked commits merge in,
A smoother stream flows.
ghost
left a comment
There was a problem hiding this comment.
This pull request introduces a significant improvement to the Helm deployer by implementing a dependency graph for releases. This addresses the issue of concurrent installations and ensures releases are deployed in the correct order. The changes also revert a previous feature that transformed imagePullPolicy when using a local cluster, which simplifies the deployment process. The code is well-structured and the tests are comprehensive. I've summarized the Google Go style guide elements referenced in this review below.
Google Go Style Guide Summary:
- Error Strings: Error strings should not be capitalized or end with punctuation (unless it's a quote or parenthetical phrase).