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

feat(helm): Add helm dependencies support #9624

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

idsulik
Copy link
Contributor

@idsulik idsulik commented Dec 20, 2024

Description
Added dependsOn to the helm release to specify dependencies, it requires when you use concurrency releases installation that was introduced here #9451

The idea for the dependsOn feature goes to @orlandoburli (#9588), happy to close this PR if you'd prefer to implement it yourself!

@idsulik idsulik requested a review from a team as a code owner December 20, 2024 07:04
@idsulik idsulik requested a review from katiexzhang December 20, 2024 07:04
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request adds support for specifying Helm release dependencies using a dependsOn field. This is particularly useful when deploying multiple Helm releases concurrently, a feature introduced in PR #9451. The intent is to ensure that releases are deployed in the correct order, preventing issues caused by dependencies not being met.

Here's a breakdown of the changes:

  • Schema Update (docs-v2/content/en/schemas/v4beta12.json):
    • Lines 2369-2383: Added the dependsOn field to the Helm release schema. This field is an array of strings, each representing the name of a Helm release that the current release depends on. A default empty array is provided.
    • Lines 2484-2497: Updated the properties array within the schema to include dependsOn.
  • Deploy Logic Update (pkg/skaffold/deploy/helm/helm.go):
    • Extensive changes (lines 261-354) were made to the Helm deployer to handle dependencies. This includes building a dependency graph, verifying for cycles, calculating deployment order, and grouping releases by dependency level to ensure correct execution order.
    • New functions calculateDeploymentOrder (lines 697-729) and groupReleasesByLevel (lines 731-757) were added to manage dependency resolution.
  • **Default Configuration Update (pkg/skaffold/schema/defaults/defaults.go):
    • Lines 20-26: Added an import for the new Helm dependency functions.
    • Lines 46-53: Modified the setHelmDefaults function to handle dependency graph verification.
    • Lines 114-143: The setHelmDefaults function now verifies the dependency graph for cycles if more than one release is defined.
  • Skaffold Configuration Update (pkg/skaffold/schema/latest/config.go):
    • Lines 1070-1076: Added the DependsOn field to the HelmRelease struct.

This PR significantly enhances the Helm deployment capabilities of Skaffold by enabling dependency management for concurrent deployments. The added dependency graph verification helps prevent deployment failures due to circular dependencies.

Let me know if you have any questions.

And now, a little haiku to lighten the mood:

Dependencies flow,
Code compiles, tests all pass,
Joyful release day!

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request introduces support for Helm dependencies using the dependsOn field. This is a valuable addition for managing complex Helm deployments. The implementation looks good overall, and addresses a clear need for managing dependencies between Helm releases within Skaffold. The changes are well-structured and follow good coding practices. The addition of dependency management is a significant improvement to Skaffold's Helm deployment capabilities.

Summary of Style Guide References:

  • Error Wrapping: Used fmt.Errorf with %w for error wrapping to preserve error context (as recommended by the Effective Go guide).
  • Function Naming: Descriptive function names like calculateDeploymentOrder and groupReleasesByLevel adhere to Go conventions.
  • Comment Clarity: Comments were added to explain complex logic and data structures.
  • Concurrency Control: The use of mutexes ensures safe concurrent access to shared resources.

README Suggestions:

The README should be updated to include documentation for the new dependsOn feature. This should include:

  • A clear explanation of the feature and its purpose.
  • Examples of how to use dependsOn in a skaffold.yaml file.
  • An explanation of how dependency resolution works.
  • A section on best practices for managing Helm dependencies with Skaffold.
  • Information on any limitations or known issues.

Additional Considerations:

  • Testing: Add comprehensive unit tests for the new functions, including BuildDependencyGraph, VerifyNoCycles, calculateDeploymentOrder, and groupReleasesByLevel. These tests should cover various scenarios, including valid and invalid dependency graphs, and ensure correct deployment order.
  • Error Handling: While the error handling is generally good, consider adding more specific error messages to provide better context in case of failures. For example, in BuildDependencyGraph, if a dependency is not found, the error message could indicate the missing release name.
  • Edge Cases: Consider testing with a larger number of releases and more complex dependency graphs to ensure robustness and identify potential performance bottlenecks.

pkg/skaffold/deploy/helm/helm.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm/helm.go Outdated Show resolved Hide resolved
Comment on lines +298 to +304
releaseNameToRelease := make(map[string]latest.HelmRelease)
for _, r := range h.Releases {
g.Go(func() error {
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
if err != nil {
return helm.UserErr(fmt.Sprintf("cannot expand release name %q", r.Name), err)
}
chartVersion, err := util.ExpandEnvTemplateOrFail(r.Version, nil)
if err != nil {
return helm.UserErr(fmt.Sprintf("cannot expand chart version %q", r.Version), err)
}
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
if err != nil {
return fmt.Errorf("cannot parse the release name template: %w", err)
}
releaseNameToRelease[releaseName] = r

Choose a reason for hiding this comment

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

low

It's good practice to check for errors after calling util.ExpandEnvTemplateOrFail, even though the function name suggests it will fail. This makes the error handling more explicit and robust. See Effective Go's section on error handling.

Suggested change
releaseNameToRelease := make(map[string]latest.HelmRelease)
for _, r := range h.Releases {
g.Go(func() error {
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
if err != nil {
return helm.UserErr(fmt.Sprintf("cannot expand release name %q", r.Name), err)
}
chartVersion, err := util.ExpandEnvTemplateOrFail(r.Version, nil)
if err != nil {
return helm.UserErr(fmt.Sprintf("cannot expand chart version %q", r.Version), err)
}
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
if err != nil {
return fmt.Errorf("cannot parse the release name template: %w", err)
}
releaseNameToRelease[releaseName] = r
releaseNameToRelease := make(map[string]latest.HelmRelease)
for _, r := range h.Releases {
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
if err != nil {
return fmt.Errorf("cannot parse the release name template: %w", err)
}
releaseNameToRelease[releaseName] = r
}

pkg/skaffold/deploy/helm/helm.go Outdated Show resolved Hide resolved
// Level 0 contains releases with no dependencies
// Level 1 contains releases that depend only on level 0 releases
// And so on...
func groupReleasesByLevel(order []string, graph map[string][]string) map[int][]string {

Choose a reason for hiding this comment

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

low

Consider adding a more detailed explanation of the leveling algorithm in the comment. This will improve understanding of the code's logic. Per the Effective Go guide, comments should provide context and explain complex algorithms.

Suggested change
func groupReleasesByLevel(order []string, graph map[string][]string) map[int][]string {
// groupReleasesByLevel groups releases by their dependency level.
// Level 0 contains releases with no dependencies.
// Level 1 contains releases that depend only on level 0 releases, and so on.
// This grouping ensures that releases are deployed in the correct order,
// respecting dependencies.

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
@idsulik idsulik force-pushed the feature-helm-dependson branch from 3fdec62 to 2ca31da Compare December 20, 2024 09:37
Signed-off-by: Suleiman Dibirov <[email protected]>
alphanota
alphanota previously approved these changes Dec 20, 2024
@alphanota
Copy link
Contributor

@idsulik Thanks for this PR. Can you please take a look at the failing unit tests? Thanks!

Signed-off-by: Suleiman Dibirov <[email protected]>
@idsulik
Copy link
Contributor Author

idsulik commented Dec 20, 2024

@alphanota pushed fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants