fix(helm): Fix helm package installation order

[idsulik]

Merge after: #9689

Description
Before version 2.14.0, there was no concurrency—each release was installed one after another in the order they appeared in the config. However, in v2.14.0, this order was broken. To enforce the correct order, dependsOn needed to be added, but the old behavior was disrupted, even though this was just a patch release.
In this PR, I changed the installation logic to respect the releases' order from the config file. if the config has dependsOn, then on each level, the order will be preserved as the releases appear in the config file, if the config hasn't dependsOn then the installation will have the old behavior(releases will be installed one after another depending on their order in the config file)

User facing changes (remove if N/A)
before: we don't preserve helm releases order when we install them
after: we preserve the order

[ghost]

Hi team,

I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, here's a summary of the changes for you and other reviewers to quickly get up to speed:

This pull request, authored by idsulik, aims to fix the installation order of Helm packages in Skaffold. Before version 2.14.0, Skaffold installed releases sequentially. Version 2.14.0 introduced a change that broke this behavior. This PR restores the correct installation order.

Here's a breakdown of the changes:

• pkg/skaffold/deploy/helm/dependencygraph.go (lines 1-198): This file introduces a new DependencyGraph struct and associated methods to manage Helm release dependencies. The key improvements are:
  • A function to create a dependency graph from a list of Helm releases, checking for duplicate names and non-existent dependencies.
  • A cycle detection function to prevent circular dependencies.
  • A function to return releases grouped by dependency level, preserving the original order within each level.
• pkg/skaffold/deploy/helm/dependencygraph_test.go (lines 1-375): Comprehensive unit tests added to verify the functionality of the DependencyGraph struct and its methods, covering various scenarios including linear dependencies, multiple dependencies, cycles, and empty graphs.
• pkg/skaffold/deploy/helm/helm.go (lines 261-343): The Helm deployer logic is updated to leverage the new DependencyGraph. The changes include:
  • Using the new NewDependencyGraph function to create a dependency graph.
  • Using GetReleasesByLevel to obtain releases grouped by dependency level.
  • Deploying releases level by level, respecting the order determined by the dependency graph.
  • Added concurrency control to allow for parallel installation of releases.
• pkg/skaffold/deploy/helm/util.go (lines 1-132): This file, which previously contained functions for building a dependency graph, cycle detection, and calculating deployment order, has been completely removed as its functionality is now handled by dependencygraph.go.
• pkg/skaffold/deploy/helm/util_test.go (lines 1-365): This file, containing unit tests for the functions in util.go, has been removed as the functionality has been moved.
• pkg/skaffold/schema/defaults/defaults.go (lines 129-141): The default setting logic for Helm deployments is updated to use the new HasCycles function to check for dependency cycles.

The PR description mentions that this change is a fix for a regression introduced in version 2.14.0, and that the new logic respects the order of releases specified in the configuration file, falling back to sequential installation if no dependencies are defined.

Let me know if you have any questions.

And now, a little haiku to lighten the mood:

Helm charts align,
Dependencies now in place,
Order is restored.

[ghost]

This pull request addresses a regression in Helm release installation order introduced in v2.14.0. The changes introduce a DependencyGraph to manage dependencies and ensure the correct installation order. The implementation looks good overall, and the tests are comprehensive. Here are a few minor suggestions based on the Effective Go style guide:

Summary of Effective Go elements referenced:

• Receivers names:
  • Short receiver names are preferred, like g for DependencyGraph.
• Package names:
  • Package names should be concise.
• Function and method names:
  • Function names should be descriptive and not include unnecessary words like Get.
• Error strings:
  • Error strings should not be capitalized and should not end with punctuation.

[alphanota]

Use year 2025 for the new files.

[alphanota]

@idsulik Thank you for this PR. From what I understand, the install order matching the order in the skaffold config file is not something that was ever explicitly stated as supported behavior, so this PR would introduce this behavior as a new feature. I think the old behavior was a consequence of how the installation logic was implemented (in sequential order), but I don't think it was ever promised behavior.

Helm also has its own mechanism to handle dependencies between charts and it supports both local and remote charts. It seems to me like that is the correct way to handle dependencies between charts.

Can you explain more your use case and why you need to keep the order, and why the dependency mechanism offered by helm doesn't fulfill your needs? I think having that info would help us better understand this PR. Thank you!

[idsulik]

Use year 2025 for the new files.

done

[idsulik]

@alphanota the helm's dependency mechanism isn't suitable for our case, because we have a different kind of packages, and generally, they don't have dependencies, but when we deploy them on our feature branch stages, we should first install rolebinding package, and only after installing other packages. we can't add the rolebinding package as a helm's dependency because it requires this dependency only for one case.
we use the same mechanism to init database so first we install package which will prepare the environment and all other packages go after it.

[idsulik]

the only way to achieve the old behavior is to use the new dependsOn, if you think it's ok

[idsulik]

@alphanota even if you don't like the idea of preserving the original order, this PR is still useful because it has refactored dependency graph code with comprehensive tests. The preserved order will help in tests. And we are not changing the official guide to tell the developers that we preserve the order

[alphanota]

@idsulik Can you please add a unit test in helm_test - we don't have a test that specifically tests the install order. I think that would be useful here.

[idsulik]

@alphanota I'll add when this PR will be merged #9689, because they have common parts and I want to avoid merge conflicts

[idsulik]

@alphanota rebased and pushed with tests

[idsulik]

deployed our project with/without dependsOn and with/without concurrency - works fine as I see

[idsulik]

@alphanota, it'd be great if you could deploy a new release as soon as you merge this PR

[alphanota]

Hey @idsulik please take a look at my comments, thanks!

[alphanota]

nit: Throughout the test for the DependsOn field we are either setting it to []string{}, nil, or omitting the field altogther when a HelmRelease is created. For consistency I would just omit the field altogether.

[idsulik]

done

[alphanota]

Instead of checking each entry individually you can take advantage of the method slices.Equal and do something like this:

			opt := cmp.Comparer(func(x, y []string) bool {
				return slices.Equal(x, y)
			})
			if diff := cmp.Diff(test.expected, graph.graph, opt); diff != nil {
				t.Errorf("%s:got unexpected diff: %s", test.name, diff)
			}

[idsulik]

done

[alphanota]

What is the behavior if this is switched to "c","b" instead of "b","c". Which order [order of appearance in releases or order of appearance in the dependsOn ] takes precendence?

[idsulik]

goog point, found an issue, fixed

[alphanota]

I think this whole comparison block would benefit from using slices.Equal method as I mentioned in comment above to make it simpler.

[idsulik]

done

[alphanota]

I don't think you should need a separate test for this, it should already be covered in TestGetReleasesByLevel.

[idsulik]

removed

[alphanota]

I think a dependency graph with cycles is invalid by default, so I don't think NewDependencyGraph should successfully return if the graph it created has cycles. I think it would be better to just call HasCycles here before successfully returning.

[idsulik]

done

[alphanota]

Continuation on NewDendencyGraph comment, if NewDependencyGraph is always called with a subsequent call to HasCycles, I would opt to just call HasCycles in NewDependencyGraph and make HasCycles package-private.

[idsulik]

done

[idsulik]

@plumpy hi! Could you please publish a new release? Want to integrate the feature into our project