Avoid --skip-refresh on local charts

[indrekj]

All the dependencies get correctly installed when dealing with remote charts.

If there's a local chart that depends on remote dependencies then those don't get automatically installed. See #526. They end up with this error:

Error: no cached repository for helm-manager-b6cf96b91af4f01317d185adfbe32610179e5246214be9646a52cb0b86032272 found. (try 'helm repo update'): open /root/.cache/helm/repository/helm-manager-b6cf96b91af4f01317d185adfbe32610179e5246214be9646a52cb0b86032272-index.yaml: no such file or directory

One workaround for that would be to add the repositories from the local charts. Something like this:

cd local-chart/ && helm dependency list $dir 2> /dev/null | tail +2 | head -n -1 | awk '{ print "helm repo add " $1 " " $3 }' | while read cmd; do $cmd; done

This however is not trivial to parse and implement.

An easier fix which I did here is just to not allow doing --skip-refresh for local repositories.

Fixes #526

[yxxhero]

@indrekj please rebase your code. then the unit test will be ok.

[yxxhero]

@indrekj is there another method that does not add a func arg?

[indrekj]

@indrekj please rebase your code. then the unit test will be ok.

I did a rebase against origin main but it still fails.

@indrekj is there another method that does not add a func arg?

Not sure. I need to know if we're dealing with a local chart and then using that we can skip the refresh. I'm not very familiar with Go. In other languages I would use keyword arguments instead of a boolean argument though. I don't think we can just skip an argument if we want to use this approach though.

[indrekj]

One idea would be to use flags... argument like in other functions and do the --skip-refresh decision before calling the function.

[yxxhero]

@indrekj please try to fix unit test. Thanks very much.

[indrekj]

There's still one test failing which I'm having trouble fixing:

    --- FAIL: TestHelmfileTemplateWithBuildCommand/oci_need (6.08s)

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,7 @@
        	            	 Building dependency release=foo, chart=$WD/temp1/foo
        	            	+Hang tight while we grab the latest from your chart repositories...
        	            	+...Unable to get an update from the "myrepo" chart repository (http://localhost:18080/):
        	            	+	Get "http://localhost:18080/index.yaml": dial tcp [::1]:18080: connect: connection refused
        	            	+...Successfully got an update from the "istio" chart repository
        	            	+Update Complete. ⎈Happy Helming!⎈
        	            	 Saving 1 charts
        	Test:       	TestHelmfileTemplateWithBuildCommand/oci_need

That test does not have anything to do with 18080 and istio. So it seems that tests are not being run completely isolated from other tests. I suspect there's some Helm cache lingering and this is causing this issue. Do you have any recommendations on what to do in this case?

[mumoshu]

Interesting- But let me rerun the test anyway, assuming it might be a flake.

[mumoshu]

Apparently it's not a flake 🤔
I remember Helmfile occasionally run helm repo up to re-fetch indices for all the local and remote chart repositories registered to the local helm installation. We might be running helm repo add istio https://... through other e2e test cases and that might be a test that looks not related to istio seems to have resulted in fetching the index for the istio charts repo.
What's uncertain for me is why it's failing now.
Could it be that the changes made in this pull request resulted in an additional helm repo up call in some code paths and revelated that our local chart repository server (that is brought up/down around the helmfile e2e test run) is broken? 🤔

[indrekj]

Could it be that the changes made in this pull request

That is likely. Previously we had --skip-refresh. The refresh seems to affect the repositories.

[yxxhero]

@indrekj @mumoshu I think the key is trying to clean up the helm repo on each iteration.

see:

helmfile/test/e2e/template/helmfile/snapshot_test.go

Line 55 in b76f1d3

for _, e := range entries {

[yxxhero]

@mumoshu @itscaro all is ok. Please review.

[yxxhero]

@indrekj could you rebase main branch?

[indrekj]

@indrekj could you rebase main branch?

I think you already did, at least I don't see any new changes in the main branch.

By the way, thank you for taking care of this. I have been swamped, and looking at the code, you did it much better than I would have.

[mumoshu]

LGTM. Thanks a lot for your contribution @indrekj @yxxhero ☺️