fix: (helm) Add expand env template for dependsOn, fix concurrent installation (#9689) #9707
Conversation
There was a problem hiding this comment.
Hi team,
I'm currently reviewing this pull request and will provide feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request addresses concurrent installation issues in the Helm deployer and improves the handling of environment variables in release names and dependencies.
Intent:
The primary goal is to fix concurrent installation problems and enhance the robustness of environment variable expansion within Helm deployments. The changes ensure that release names and dependsOn fields correctly handle environment variables, preventing unexpected behavior during concurrent deployments.
Changes:
pkg/skaffold/deploy/helm/helm.go: Modified the deployment logic to handle concurrency more effectively. Specifically, lines 288-298 were changed to allow for configurable concurrency, with a default of sequential execution. The handling of release names (lines 338-344) now includes environment variable expansion for release names. The logging (lines 309-312) was also slightly adjusted for clarity.pkg/skaffold/deploy/helm/helm_test.go: Added a new test case (testDeployWithDependsOnConfig) on lines 69-82 to verify the correct functionality ofdependsOnwith environment variables. This involved adding a new test case to theTestHelmDeployfunction (lines 464-481) to cover the new scenario.pkg/skaffold/deploy/helm/util.go: Removed unnecessary environment variable expansion from theBuildDependencyGraphfunction (lines 27-29). This simplifies the dependency graph creation and removes a potential source of errors.pkg/skaffold/deploy/helm/util_test.go: Updated a test case description (lines 69-79) to reflect the changes in theBuildDependencyGraphfunction.
Activity:
The pull request has been updated with additional tests to cover the changes made. The author, Suleiman Dibirov, has signed off on the changes.
I'll provide a more detailed review shortly. Thanks!
And now, a little haiku to brighten your day:
Code flows like a stream,
Fixes merge, the build turns green,
Joyful, smooth release.
There was a problem hiding this comment.
This pull request addresses a concurrency issue in the Helm deployer and allows for environment variable expansion in dependsOn. The changes are well-structured and improve the functionality and flexibility of the Helm deployer. The added test case effectively covers the new dependsOn behavior. A few minor suggestions for improvement are listed below.
I've referenced the Effective Go style guide in this review.
…tallation (GoogleContainerTools#9689) * fix(helm): Add expand env template for dependsOn and fix concurrent installation * tests Signed-off-by: Suleiman Dibirov <[email protected]> --------- Signed-off-by: Suleiman Dibirov <[email protected]>
fix(helm): Add expand env template for dependsOn and fix concurrent installation
tests
Signed-off-by: Suleiman Dibirov [email protected]
Signed-off-by: Suleiman Dibirov [email protected]