fix: add install-helm dependency to helm-lint target

[AutuSnow]

The helm-lint target was calling $(HELM) without ensuring helm is installed first. This caused CI failures when helm was not available in the system PATH.

Other helm targets (helm-package, helm-push) already depend on install-helm. This change makes helm-lint consistent with them.

Fixes the error:
bash: /home/runner/work/.../bin/helm: No such file or directory
make: *** [Makefile:248: helm-lint] Error 127

[fmuyassarov]

I think the issue isn't about availability of helm, because ubuntu-latest does come with Helm pre-installed. The issue seems to be about the path of the Helm

[fmuyassarov]

In other words, we are expecting the Helm to be available on the local bin directory while it is on the system-wide bin directory

[fmuyassarov]

If we override the Helm path during the action, then that should be enough I think. Also, we could drop

dra-driver-cpu/.github/workflows/helm-lint.yaml

Line 20 in ebd63d3

- name: Install Helm

[AutuSnow]

@fmuyassarov Understood. Two options:

1: Override HELM variable

Change line 33 to HELM ?= helm so it uses system helm by default, and only installs locally if not found:


makefile
HELM ?= helm

.PHONY: install-helm
install-helm: $(OUT_DIR)
@if ! command -v $(HELM) >/dev/null 2>&1; then \
echo "Helm not found, installing to $(OUT_DIR)..."; \
GOBIN=$(OUT_DIR) go install helm.sh/helm/v3/cmd/helm@$(HELM_VERSION_SHA); \
fi


This way CI uses the preinstalled helm, and we can remove the azure/setup-helm step.

2: Symlink

Keep HELM = $(OUT_DIR)/helm but symlink system helm if available.

I think Option 1 matches what you suggested - does that work?

[fmuyassarov]

@fmuyassarov Understood. Two options:  1: Override HELM variable  Change line 33 to HELM ?= helm so it uses system helm by default, and only installs locally if not found: 

makefile
HELM ?= helm

.PHONY: install-helm
install-helm: $(OUT_DIR)
@if ! command -v $(HELM) >/dev/null 2>&1; then \
echo "Helm not found, installing to $(OUT_DIR)..."; \
GOBIN=$(OUT_DIR) go install helm.sh/helm/v3/cmd/helm@$(HELM_VERSION_SHA); \
fi

 This way CI uses the preinstalled helm, and we can remove the azure/setup-helm step.  2: Symlink  Keep HELM = $(OUT_DIR)/helm but symlink system helm if available.  I think Option 1 matches what you suggested - does that work?

I'm afraid this will actually break. If Helm isn’t available, we install it into the local bin, but the system won’t search that path. One example is cloud build, during the new release we also plan to package and upload the chart artifact. Since the cloudbuild environment doesn’t come with Helm preinstalled, we’ll need to install it at runtime and unless the local bin directory is added to PATH the Helm binary won’t be discoverable.

[AutuSnow]

You're right - if HELM ?= helm and we install to bin/helm, it won't be found since bin/ isn't in PATH.

keep HELM = $(OUT_DIR)/helm, but make install-helm smarter:


HELM = $(OUT_DIR)/helm

.PHONY: install-helm
install-helm: $(OUT_DIR)
@if [ -f "$(HELM)" ]; then \
exit 0; \
elif command -v helm >/dev/null 2>&1; then \
ln -sf "$$(command -v helm)" "$(HELM)"; \
else \
echo "Installing helm to $(OUT_DIR)..."; \
GOBIN=$(OUT_DIR) go install helm.sh/helm/v3/cmd/helm@$(HELM_VERSION_SHA); \
fi

This way:
- CI (ubuntu-latest): symlinks system helm to bin/helm
- cloudbuild: installs to bin/helm
- Always uses $(OUT_DIR)/helm consistently

We can still remove azure/setup-helm from the workflow since ubuntu-latest has helm preinstalled.

[AutuSnow]

@fmuyassarov Does this align with your ideas?

[fmuyassarov]

@fmuyassarov Does this align with your ideas?

Sorry for late response @AutuSnow .

I just realized that it might actually be fine to add the Helm installation target as a dependency for helm-lint as well, since installing it locally into bin does not really hurt.
The only thing that felt slightly not good to me is that the runner image already comes with Helm preinstalled, so in that case the installation step is technically unnecessary. But the overhead is very smal most probably and I also would not want to overcomplicate this change.

My preference would probably be to keep it simple and maybe improve the current approach just a little bit on top of what you already have now. In other words,

diff --git a/Makefile b/Makefile
index e0e7491..e8daf0c 100644
--- a/Makefile
+++ b/Makefile
@@ -30,7 +30,7 @@ KIND_K8S_VERSION ?= v1.36.0
 # paths
 YQ = $(OUT_DIR)/yq
 GOLANGCI_LINT = $(OUT_DIR)/golangci-lint
-HELM = $(OUT_DIR)/helm
+HELM := $(or $(shell command -v helm 2>/dev/null),$(OUT_DIR)/helm)

 # disable CGO by default for static binaries
 CGO_ENABLED=0
@@ -241,7 +241,7 @@ install-yq: $(OUT_DIR)  ## make sure the yq tool is available locally
 install-golangci-lint: $(OUT_DIR) ## make sure the golangci-lint tool is available locally
        @hack/fetch-golangci-lint.sh $(OUT_DIR) $(GOLANGCI_LINT_VERSION)

-helm-lint: ## lint helm chart with strict mode
+helm-lint: install-helm ## lint helm chart with strict mode
        $(HELM) lint --strict ${HELM_CHART}

this way we avoid installing the Helm on the runner too. WDYT?

[AutuSnow]

@fmuyassarov Sure this is the least amount of modification, and I think this method has finer granularity than the one I used above

[fmuyassarov]

Thanks.
/lgtm

[AutuSnow]

@ffromani @pravk03 Could you please help me review this PR when you have time

[pravk03]

/lgtm
/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AutuSnow, pravk03

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [pravk03]

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