Implements DependencyUpdate for helm charts #2135

0xavi0 · 2024-02-08T15:19:54Z

Updates dependencies in helm charts by default.
A new flag disableDependencyUpdate is added to the fleet.yaml file in order to disable the feature.

If you don't specify anything, dependencies will be updated.
In order to avoid dependency updates, use disableDependencyUpdate: true in the fleet.yaml file.

A new package helmupdater is added to implement the dependencies update.
The implementation is based on helm's when ussing the --dependency-update flag in the install command.

Dependencies are applied to the bundle (upstream) so they're resolved already when applying downstream.

Note. It also adds a new step when running e2e tests, because the new dependency update tests rely on both, the helm-registry and the git-server

Refers to: #1672

A new flag `disableDependencyUpdate` is added to the `fleet.yaml` file in order to disable the feature, which is active by default. A new package `helmupdater` is added to implement the dependencies update. The implementation is based on helm's when ussing the `--dependency-update` flag in the install command. Dependencies are applied to the bundle (upstream) to they're resolved already when applying downstream. Refers to: rancher#1672 Signed-off-by: Xavi Garcia <[email protected]>

weyfonk

Nice work, thanks for this!
Leaving a few comments, mostly nitpicks.

.github/workflows/e2e-ci.yml

internal/bundlereader/loaddirectory.go

internal/helmupdater/helmupdater.go

integrationtests/cli/apply/apply_test.go

weyfonk · 2024-02-12T16:31:29Z

integrationtests/cli/apply/apply_test.go

+				presentInBundleResources(path.Join(tmpDir, "Chart.lock"), bundle.Spec.Resources)
+				presentInBundleResources(path.Join(tmpDir, "charts/config-chart-0.1.0.tgz"), bundle.Spec.Resources)
+
+				return !bundle.Spec.KeepResources


Why is KeepResources relevant here? Would this be a leftover from reusing specs of already existing test cases? (same question about the test description)

It is not, indeed. I've deleted that.

integrationtests/cli/apply/apply_test.go

e2e/single-cluster/helm_dependencies_test.go

Signed-off-by: Xavi Garcia <[email protected]>

internal/helmupdater/helmupdater.go

Signed-off-by: Xavi Garcia <[email protected]>

Changed from disableDependencyUpdate to disableDepsUpdate Signed-off-by: Xavi Garcia <[email protected]>

weyfonk · 2024-02-13T15:26:19Z

integrationtests/cli/apply/apply_test.go

+				return true
+			}).Should(BeTrue())


I think this doesn't make much sense: either we should return an expression, which may or may not evaluate as true, or skip this altogether: do we even need Eventually blocks here?

I thought that was because we don't know when the Bundle is going to be ready so Eventually waits up to its timeout (5 minutes) for success.
It would only reach that return true statement when all the statements before that one are successful (at least that's what I got from the tests I did)
As there are quite a few files to check I thought it was better to do that than returning a chain of booleans.
(This file is found and that file is found and... etc)

Given I'm new to Ginkgo I guess there are better ways of doing what I need... I'll investigate. :/

Since the test uses the cli to create the bundle it will be ready when the cli exists. The buffer buf will be in it's final state, as soon as cli.GetBundleFromOutput returns. Eventually is great when we're waiting for resources to appear in k8s. If I understood that correctly, I'd definitely remove them.

I also see a lot of helper functions like onlyPresentInBundleResources, these could be custom matchers: https://onsi.github.io/gomega/#adding-your-own-matchers
But I don't think we're using those anywhere else, no need to do change it.

I've tested deleting the Eventually statements and running in a loop for 10000 times and it passed.
So... Eventually statements deleted.

I've also added a couple of custom matchers and deleted the helpers.

Also... I've changed the already existing tests to use the custom matchers and removed the Eventually statements to avoid future confusions and make the whole file consistent.

weyfonk

Thanks for converting existing test cases to using matchers, they're much more readable now!

weyfonk · 2024-02-16T14:55:08Z

integrationtests/cli/apply/apply_test.go

+			Expect(len(root.Spec.Resources)).To(Equal(5))
+
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))


Aren't these two the same check? You have converted it correctly, but I suspect there was a typo in the test case in the first place, eg.:

Suggested change

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/deployment.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))

(disclaimer: I haven't tested this)

weyfonk · 2024-02-16T14:56:43Z

integrationtests/cli/apply/apply_test.go

+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentC/fleet.yaml").To(bePresentInBundleResources(deploymentC.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))


Same doubt as above:

Suggested change

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/deployment.yaml").To(bePresentInBundleResources(root.Spec.Resources))

weyfonk · 2024-02-16T15:02:24Z

integrationtests/cli/apply/apply_test.go

+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentB/svc.yaml").To(bePresentInBundleResources(root.Spec.Resources))
+			Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentC/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))


I believe one check is missing from the previous version:

Suggested change

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentC/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentC/fleet.yaml").To(bePresentInBundleResources(root.Spec.Resources))

Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentD/deployment.yaml").To(bePresentInBundleResources(root.Spec.Resources))

Eventually is not needed when testing for resources in bundles as we get the bundle from `cli.GetBundleListFromOutput` and the buffer read will be in its final state once `cli.GetBundleListFromOutput` returns. Also addind custom Gomega matchers to avoid using helper functions and to get the exact line when any test assertion fails. Already existing tests have been changed to use the custom matchers and don't use Eventually, also. Signed-off-by: Xavi Garcia <[email protected]>

0xavi0 requested a review from a team as a code owner February 8, 2024 15:19

0xavi0 force-pushed the 1672-depedency-update branch 8 times, most recently from 331ccd3 to c57393b Compare February 9, 2024 11:09

0xavi0 force-pushed the 1672-depedency-update branch from c57393b to 999dc47 Compare February 9, 2024 11:35

0xavi0 self-assigned this Feb 9, 2024

0xavi0 added this to the v2.9.0 milestone Feb 9, 2024

weyfonk reviewed Feb 13, 2024

View reviewed changes

Changes after code-review

702ba99

Signed-off-by: Xavi Garcia <[email protected]>

0xavi0 requested a review from weyfonk February 13, 2024 13:03

manno reviewed Feb 13, 2024

View reviewed changes

internal/helmupdater/helmupdater.go Outdated Show resolved Hide resolved

Change function name to make it go-idiomatic

527b259

Signed-off-by: Xavi Garcia <[email protected]>

0xavi0 requested a review from manno February 13, 2024 14:15

kkaempf added kind/enhancement area/helm labels Feb 13, 2024

Change parameter name to make it shorter

6a471d4

Changed from disableDependencyUpdate to disableDepsUpdate Signed-off-by: Xavi Garcia <[email protected]>

weyfonk reviewed Feb 13, 2024

View reviewed changes

0xavi0 requested a review from weyfonk February 16, 2024 12:42

0xavi0 force-pushed the 1672-depedency-update branch from 57f45d9 to 70fb948 Compare February 16, 2024 14:28

weyfonk reviewed Feb 16, 2024

View reviewed changes

0xavi0 force-pushed the 1672-depedency-update branch from 70fb948 to 28c9c5b Compare February 16, 2024 15:55

weyfonk approved these changes Feb 16, 2024

View reviewed changes

0xavi0 merged commit 3ad25e1 into rancher:master Feb 16, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements DependencyUpdate for helm charts #2135

Implements DependencyUpdate for helm charts #2135

0xavi0 commented Feb 8, 2024 •

edited

Loading

weyfonk left a comment

weyfonk Feb 12, 2024

0xavi0 Feb 13, 2024

weyfonk Feb 13, 2024

0xavi0 Feb 13, 2024

manno Feb 14, 2024

0xavi0 Feb 16, 2024 •

edited

Loading

weyfonk left a comment

weyfonk Feb 16, 2024

weyfonk Feb 16, 2024

weyfonk Feb 16, 2024

	Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/fleet.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))
	Expect(cli.AssetsPath + "nested_mixed_two_levels/nested/deploymentA/deployment.yaml").To(bePresentInBundleResources(deploymentA.Spec.Resources))

Implements DependencyUpdate for helm charts #2135

Implements DependencyUpdate for helm charts #2135

Conversation

0xavi0 commented Feb 8, 2024 • edited Loading

weyfonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xavi0 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

weyfonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xavi0 commented Feb 8, 2024 •

edited

Loading

0xavi0 Feb 16, 2024 •

edited

Loading