feat(helm): adding helm-diff target

[hjoshi123]

Motivation: cert-manager/cert-manager#8183

CyberArk tracker: VC-46541

[mladen-rusev-cyberark]

/cybr

[inteon]

helm_chart_image_registry probably also needs to be defined.

[hjoshi123]

https://github.com/hjoshi123/makefile-modules/blob/1dcfe9cc44ee5b1240b21962db3da06c64e1378a/modules/helm/helm.mk#L41 I think it is defined already here

[inteon]

Is cert-manager the chart name? could you use a variable for that too?

[hjoshi123]

Oh right.. yes I can do that

[maelvls]

I struggled a little with trying to run this PR from within cert-manager. I found that the helm-diff target isn't reachable from within the cert-manager project:

# From the cert-manager project:
yq -i '(.targets["make/_shared"][] , .targets["make/_shared_new"][]) |= (.repo_ref = "refs/pull/470/head")' klone.yaml
make upgrade-klone

$ make helm-diff
make: *** No rule to make target `helm-diff'.  Stop.

That's because we only import crds.mk in the cert-manager project since we don't use the helm makefile-modules for release the helm chart yet.

In trust-manager, I had the following:

# From the trust-manager project:
yq -i '(.targets["make/_shared"][] , .targets["make/_shared_new"][]) |= (.repo_ref = "refs/pull/470/head")' klone.yaml
make upgrade-klone

$ make helm-diff    
make/_shared/helm///helm.mk:193: *** missing separator.  Stop.

Looks like this new target won't work in cert-manager until we are done with cert-manager/cert-manager#7718.

But it will work for all of the other projects such as trust-manager, so I'm in favor of adding it once the above error is fixed.

Great work!

[maelvls]

@hjoshi123 Hey, do you need any help on this?

[hjoshi123]

@maelvls sorry was traveling for kubecon and then the conference so got distracted. Will fix it this weekend.

[hjoshi123]

@maelvls can you try running it now? I missed a : in the PHONY which would cause that error

[maelvls]

In make, each command is run in its own sub-shell, so these variables won't be set in the $(HELM) template below. For example, in trust-manager, I'm seeing:

# From the trust-manager project:
$ yq -i '(.targets["make/_shared"][] , .targets["make/_shared_new"][]) |= (.repo_ref = "refs/pull/470/head")' klone.yaml
$ make upgrade-klone
$ make helm-diff
make/_shared/helm///helm.mk:196: warning: undefined variable `OUTPUT_DIR'

One way is to continue the command with backslashes:

target:
	@TMP_OLD=$$(mktemp -d); \
	@TMP_NEW=$$(mktemp -d); \
	@OUTPUT_DIR=$$(mktemp -d); \
    $(HELM) template $(helm_chart_oci_name) ...

Another way is to use $(eval export ...). It is a little more convenient as this method allows you to keep set that var for all subsequent commands:

target:
	$(eval export TMP_OLD=$$(mktemp -d))
	$(eval export TMP_NEW=$$(mktemp -d))
	$(eval export OUTPUT_DIR=$$(mktemp -d))
    $(HELM) template $(helm_chart_oci_name) ...

[hjoshi123]

Ah I didnt know this.. Thank you for pointing that out @maelvls.. still learning to write proper makefiles 😅

[hjoshi123]

Also @maelvls had a thought should we somehow set the old version i.e. helm_chart_old_version to latest release? Currently its user driven

[maelvls]

Oops, i might have given you the wrong command, I'm now getting:

$ make helm-diff
make/_shared/helm///helm.mk:196: warning: undefined variable `mktemp -d'
make/_shared/helm///helm.mk:196: warning: undefined variable `mktemp -d'
make/_shared/helm///helm.mk:196: warning: undefined variable `mktemp -d'
make/_shared/helm///helm.mk:196: warning: undefined variable `mktemp -d'
Usage: make helm-diff helm_chart_old_version=<version>
make: *** [helm-diff] Error 1

I guess it could be:

Suggested change

	$(eval export OUTPUT_DIR=$$(mktemp -d))
	$(eval export TMP_OLD=$(shell mktemp -d))
	$(eval export TMP_NEW=$(shell mktemp -d))
	$(eval export OUTPUT_DIR=$(shell mktemp -d))

[maelvls]

we could somehow set the old version i.e. helm_chart_old_version to latest release? Currently its user driven

I agree, having helm_chart_old_version automatically be figured out would be great!

[maelvls]

@hjoshi123 I'll unassign myself from this PR for now as I get asked "what's the progress on this" every day in our internal standups 😅 But I can continue helping you on this one 👍

[hjoshi123]

@maelvls sorry for the delay. With holidays this week, I couldn't get to it. But yes I will try to make the changes and work with you to get this merged. Thank you for helping me out on this.

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign inteon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hjoshi123]

@maelvls how does this look now? I did ponder on the helm old release and its not straight forward to fetch those.. I did come up with a shell script which could do that (pinged you on slack with it) but I feel that would make this PR polluted. We could do that as a follow-up. WDYT?