Unit test overhaul and handling additional PackagePush and Lint function use cases #70

frizzr · 2025-03-01T17:18:39Z

I added the Chart struct to help with, for example, modifying your test Helm chart(s) dynamically. This was necessary since by supplying the optional PackagePush setVersionTo and setAppVersionTo parameters, it would change the test charts' content and it was just easier to dynamically make those modifications.

…sion.

chrira

Thanks for the tests!
I have some errors running dagger -m tests/ call all

./main.go:140:145: unknown field Version in struct literal of type dagger.HelmPackagePushOpts                                                                                                  
./main.go:169:145: unknown field AppVersion in struct literal of type dagger.HelmPackagePushOpts

You are not having problems?
Sometime I have testing issues because of caching.

helm/main.go

frizzr · 2025-03-06T12:49:02Z

You are not having problems?
Sometime I have testing issues because of caching.

@chrira :

My apologies for those issues.

I am having problems running unit tests in my local environment....in our company we have to use a custom runner for Dagger due to needing to setup certificates for our company proxy.

RIght now we use Dagger v0.14.0 with our customized base image while your unit tests use v0.16. Do you have any general Dagger advice for handling different environments like this?

I know you are working on setting up CI here....would it be possible to get a minimal CI setup going....just enough to at least run the unit tests upon any change? While it is not ideal to go back to 'push and pray,' I may have to. :(

chrira · 2025-03-19T05:47:56Z

@frizzr no worries!

Yes, the tests are executed in GitHub.
The module tests should run on your fork. The integration test will not because of missing credentials.

Here is one example on your fork: https://github.com/frizzr/dagger-module-helm/actions/runs/13639799085/job/38126851023

Hope this helps you getting forward.

frizzr · 2025-04-22T18:36:23Z

Hello @chrira! It has been awhile, but I finally finished a complete rework of the unit tests along with my feature adds. I'm happy to say that it is all safe to execute the tests in parallel and I came up with a way to guarantee each test is completely independent of each other by having a set of Chart structs that represents our testdata charts in tests/testdata.

This struct, when instantiated as part of NewChartSession, will automatically rewrite each testdata chart's Chart.yaml inside the container's context and automatically suffix a random string to the end of the chart name (without changing the host's content.)

With a different set of chart names per test, it guarantees that we can safely perform tests using ttl.sh and all of this implementation is located in tests/charts.go hidden away from the actual unit tests. The unit tests of course have access to what the new chart name is within each test.

This was a LOT of work I know, and I hope it isn't too crazy or too drastic of a change. Still, I think it could solve some things now and into the future, but I'll let you be the judge of that.

If the whole thing is "too much", then I could fall back and move my feature adds across to the other PR and leave the unit tests alone.

Thanks for considering!

helm/main.go

chrira · 2025-04-23T14:04:28Z

@frizzr what a work, thank you!

Unfortunately it breaks our main return logic of the PackagePush method. See comment:
https://github.com/puzzle/dagger-module-helm/blob/main/helm/main.go#L65

As @RetGal mentioned, the test method for this logic on the main branch (helm-package-push-with-existing-chart) fails with your changes.

Changing fmt.Errorf("Debug") back to nil and re-adding the test should fix it.

frizzr · 2025-04-28T10:27:52Z

@chrira and @RetGal :

These are hopefully the last set of changes!

I added a good bit more logic, including a use case I had to handle immediately for both PackagePush and Lint where we need to login to more than one registry to be able to resolve all dependencies in a chart. The added logic will scan all dependencies in the chart and for PackagePush if any registries differ from the registry specified, it will login to that registry with the already-supplied credentials in --username and --password. This also meant I needed to optionally accept --username and --password for the Lint function (but I obviously didn't need to add registry/repository arguments to Lint.)

Although it sounds limiting to assume that the same set of credentials can login to multiple different registries, this limitation is acceptable to me given we use the same JFrog Artifactory instance for all of our Helm repositories, but we still need to login to each repository in our dependencies. I would think my situation would be fairly common, so I felt the limitation is acceptable in general.

I also changed the fmt.Errorf("Debug") back to nil.

I did have to make a (breaking?) change to the PackagePush function: For things to work properly with non-OCI repositories, I had to add a new parameter for the non-OCI ability to push a Helm chart to a particular subpath in a Helm repository and change the documentation accordingly....so the user will no longer put the subpath in as part of the repository parameter.

The big question I have is....how did my non-OCI unit test ever work? Wouldn't we need to simulate a non-OCI repository to do the test (given that ttl.sh only can do OCI repositories?) For now I have commented out that unit test.

RetGal · 2025-04-29T14:25:18Z

@frizzr you're right, the non-OCI test probably never worked.
However, this fact was hidden because curl was not called with the -f (fail) flag. We also have an integration test that is executed in the gh action, but only for OCI - in the absence of a non-OCI registry.

@chrira we should probably always call curl with the fail flag.

…her image.

RetGal · 2025-04-30T07:55:30Z

@frizzr since you are the only person we know who uses this module along with a non-oci registry and we currently have no means to unit test it, i'd propose to merge it, if it works for your real life use case.

@chrira any objections from your side?

chrira

@frizzr are you still working on it?
Note when you are finished. Then we will do the final review and merge it.
Thanks for contributing!

chrira · 2025-04-30T17:16:50Z

helm/main.go

@@ -12,18 +12,23 @@ import (
 	"fmt"
 	"os"
 	"strings"
+    "hash/crc32"


fix formatting

chrira · 2025-04-30T17:18:00Z

helm/main.go

+	Registry   		string `yaml:"registry"`
+	Repository 		string `yaml:"repository"`
+	Oci        		bool   `yaml:"oci"`
+	NonOCISubpath	string `yaml:"nonOciSubpath"`


fix alignment (check tabs/spaces)

chrira · 2025-04-30T17:20:59Z

helm/main.go

+		Registry:		registry,
+		Repository:		repository,
+		Oci:			!useNonOciHelmRepo,
+		NonOCISubpath:	nonOciRepoSubpath,


fix alignment (check tabs/spaces)

chrira · 2025-04-30T17:22:18Z

helm/main.go

-		WithWorkdir("/helm")
-	version, err := c.WithExec([]string{"sh", "-c", "helm show chart . | yq eval '.version' -"}).Stdout(ctx)
+	c := h.createContainer(directory)
+	//c := dag.Container().


please remove commented code

chrira · 2025-04-30T17:26:28Z

helm/main.go

@@ -197,7 +293,8 @@ func (h *Helm) Test(
 	args []string,
 ) (string, error) {
 	c := h.createContainer(directory)
-	out, err := c.WithExec([]string{"sh", "-c", fmt.Sprintf("%s %s", "helm-unittest", strings.Join(args, " "))}).Stdout(ctx)
+	out, err := c.WithExec([]string{"sh", "-c",
+										fmt.Sprintf("%s %s", "helm-unittest", strings.Join(args, " "))}).Stdout(ctx)


check formatting

chrira · 2025-04-30T17:29:28Z

helm/main.go

-			WithExec([]string{"sh", "-c", `echo ${REGISTRY_PASSWORD} | helm registry login ${REGISTRY_URL} --username ${REGISTRY_USERNAME} --password-stdin`}).
-			WithoutSecretVariable("REGISTRY_PASSWORD").
-			Sync(ctx)
+		// We assume we are already logged into an OCI registry


why is here no login needed?
@RetGal do we have Integration tests for the case that the version exists on the registry?

chrira · 2025-04-30T17:31:15Z

helm/main.go

 }

-func (h *Helm) hasMissingDependencies(
+func (h *Helm) queryChartWithYq(


chrira · 2025-05-12T14:38:23Z

@frizzr
Please run gofmt -w helm/main.go on your go files. We formatted the code on main.
Then you should be able to rebase your branch with main.
We are still struggling with testing, see #84
Thanks

frizzr · 2025-05-14T12:08:59Z

Sorry for the delays. I am getting laid off from my job effective late next month, but I do want to keep working on this even after my job ends. The main problem that I am having right now is very specific to our CI environment. The code as-is works perfectly except in our 'docker-in-docker' environment...we run Actions Runner Controller in a K8s cluster and I am having a bizarre issue with `helm repo add` not keeping state...the very next `WithExec` command I run completely loses the dot config directory for Helm...but in all other environments it seems to work great. I'll post more info later. Thanks!

…

On Mon, May 12, 2025, 10:38 Christoph Raaflaub ***@***.***> wrote: *chrira* left a comment (puzzle/dagger-module-helm#70) <#70 (comment)> @frizzr <https://github.com/frizzr> Please run gofmt -w helm/main.go on your go files. We formatted the code on main. Then you should be able to rebase your branch with main. We are still struggling with testing, see #84 <#84> Thanks — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGKTEYEQWTHZW7WPKB5ROD26CW7LAVCNFSM6AAAAABYEJGGYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZSHA2DENZUGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

frizzr added 7 commits February 28, 2025 07:23

Add optional setting for chart version and appVersion

20d52e4

Added optional params to opts struct.

e874348

Added public methods Name and AppVersion to get chart name and appVer…

2f1589e

…sion.

Added the Chart struct to make unit tests easier.

7e12a92

Fixed bugs.

2036e13

Fixed bug.

ee4f847

Fixed another bug.

11bde94

chrira requested changes Mar 5, 2025

View reviewed changes

helm/main.go Outdated Show resolved Hide resolved

helm/main.go Outdated Show resolved Hide resolved

frizzr added 3 commits April 18, 2025 07:51

Formatting fixes.

782685f

checkpoint

7ebbea1

Finally finished refactor of test structure and all tests pass

3e5ff21

frizzr requested a review from chrira April 23, 2025 11:02

RetGal reviewed Apr 23, 2025

View reviewed changes

helm/main.go Outdated Show resolved Hide resolved

frizzr added 2 commits April 26, 2025 09:13

Checkpoint for debugging.

5029e56

All unit tests pass (but the non-OCI case, which is commented out.)

b737792

frizzr requested a review from RetGal April 28, 2025 11:10

frizzr mentioned this pull request Apr 28, 2025

Add optional parameters to PackagePush for chart version and appVersion #69

Closed

frizzr changed the title ~~Alternate version of #69: Same as #69 + unit tests + unit test Chart struct~~ Unit test overhaul and handling additional PackagePush and Lint function use cases Apr 28, 2025

frizzr added 7 commits April 28, 2025 07:15

Edited documentation to reflect changes.

68d381f

Added helm repo update to non-OCI registry login.

dda3178

Fix bug.

34eee68

Added --pass-credentials to helm repo add.

50f03ce

Changed helm dep update in Lint to match PackagePush.

ea4e92d

Debug.

fe0dd01

Added debug.

2b4d490

frizzr added 4 commits April 29, 2025 06:33

Debug 2

36d12ca

It works...finally.

1c29ab4

Bug fix for non-oci package push

20dadd4

Changed base container for package-push with createContainer

e424b7a

frizzr added 4 commits April 29, 2025 10:34

Reverted curl commands back to pre version 8 to be compatible with ot…

3b5f984

…her image.

Added return value to debug wrong state.

108d1d3

Checkpoint

acb8e8d

Checkpoint

0bcad65

Add sync for debugging missing helm state change.

caef739

chrira requested changes Apr 30, 2025

View reviewed changes

frizzr added 13 commits May 2, 2025 11:08

Debugging

46c061d

More debug

14b32b3

fixes

fd6e85c

Fix.

2c658a0

Bust dagger cache.

64f9e72

Added bust cache in different places.

f8f151d

Fix.

fbc09a8

Another fix.

5201dc4

Updating to v0.18.5 for better debugging and results.

897a89b

Debug via directory digests.

8a06281

More debug.

0eb956c

Trying out a potential fix.

149e878

Fix for package push

022b755

Unit test overhaul and handling additional PackagePush and Lint function use cases #70

Are you sure you want to change the base?

Unit test overhaul and handling additional PackagePush and Lint function use cases #70

Uh oh!

Conversation

frizzr commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

frizzr commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrira commented Mar 19, 2025

Uh oh!

frizzr commented Apr 22, 2025

Uh oh!

Uh oh!

chrira commented Apr 23, 2025

Uh oh!

frizzr commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RetGal commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RetGal commented Apr 30, 2025

Uh oh!

chrira left a comment

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

chrira commented May 12, 2025

Uh oh!

frizzr commented May 14, 2025 via email

Uh oh!

Uh oh!

frizzr commented Mar 1, 2025 •

edited

Loading

frizzr commented Mar 6, 2025 •

edited

Loading

frizzr commented Apr 28, 2025 •

edited

Loading

RetGal commented Apr 29, 2025 •

edited

Loading