Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not error for remote chart if there's a missing local repo cache #11963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikeseese
Copy link

@mikeseese mikeseese commented Apr 3, 2023

What this PR does / why we need it:
This PR closes #11961 by continuing to scanReposForURL if one of the local repos doesn't have a cache rather than erroring. This PR also adds the hint to helm repo update to ErrNoOwnerRepo to catch cases that are expecting to find it in the local cache (i.e. u := localrepo/cache).

Ultimately, this makes it so that a full HTTP URL for the chart will never fail because in chart_downloader.go it will proceed to downloading it if scanReposForUrl returns ErrNoOwnerRepo (line 216):

If there is no special config, return the default HTTP client and swallow the error.

This PR also adds a test that tests for the case where there's a non-empty repository config but an empty repository cache.

Special notes for your reviewer:
I verified this actually fixes #11961 by using the reproduction steps listed there compared with the released/installed version of helm and the manually built version located in bin/helm (after running make build). Here was the final output of that:

/mnt/d/work/helm$ helm template traefik --repo https://helm.traefik.io/traefik
Error: no cached repo found. (try 'helm repo update'): open /home/seese/.cache/helm/repository/ingress-nginx-index.yaml: no such file or directory
/mnt/d/work/helm$ bin/helm template traefik --repo https://helm.traefik.io/traefik
---
# Source: traefik/templates/rbac/serviceaccount.yaml
kind: ServiceAccount
apiVersion: v1
metadata
<...>

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 3, 2023
@mikeseese mikeseese marked this pull request as ready for review April 3, 2023 20:49
@joejulian joejulian added this to the 3.12.0 milestone Apr 4, 2023
@mikeseese mikeseese marked this pull request as draft April 6, 2023 20:06
@mikeseese
Copy link
Author

I'm moving this back to draft to add in some minor changes to increase performance (as suggested by @MichaelMorrisEst in #11961).

@mikeseese
Copy link
Author

Looking at the comments in chart_downloader.go, I think the original intention of scanReposForURL is to handle more than just finding the first repo in repositories.yaml that has the same URL. I don't have time to ensure the changes are not introducing a regression, so I'm leaving the PR as is. This ensures the least amount of regressions, is easier to grok/approve, requires less tests, solves my specific use cases, etc. in favor of leaving "making scanReposForURL" more performant for another, more determined contributor.

@mikeseese mikeseese marked this pull request as ready for review April 7, 2023 00:05
@joejulian joejulian modified the milestones: 3.12.0, 3.13.0 May 5, 2023
@mattfarina mattfarina modified the milestones: 3.13.0, 3.14.0 Sep 25, 2023
@mattfarina mattfarina modified the milestones: 3.14.0, 3.15.0 Mar 13, 2024
Copy link

@MissedTheMark MissedTheMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good, as does the new test.

Manually testing the change locally shows that it works correctly.
I have also verified that the new test fails on the old code, and passes with this change.

New test

Test failing on old code:

SCR-20240510-maac

Test passing on new code:

SCR-20240510-lzib

Manually testing locally

Before this change:

After adding a repo (ingress-nginx) then clearing the Helm cache (note the unexpected reference to ingress-nginx-index.yaml):

$ helm template traefik --repo https://helm.traefik.io/traefik
Error: no cached repo found. (try 'helm repo update'): open /Users/mark/Library/Caches/helm/repository/ingress-nginx-index.yaml: no such file or directory

After this change:

After adding a repo (ingress-nginx) then clearing the Helm cache:

$ helm template traefik --repo https://helm.traefik.io/traefik
---
# Source: traefik/templates/rbac/serviceaccount.yaml
kind: ServiceAccount
apiVersion: v1
metadata:
  name: release-name-traefik
 ... etc

@MissedTheMark
Copy link

@mikeseese Looks like there's some CI issue, might be worth a merge from main?

Signed-off-by: Mike Seese <seesemichaelj@gmail.com>
@mikeseese
Copy link
Author

Thanks @MissedTheMark for the review! I just rebased off of main. Let me know if there are any other issues.

@scottrigby scottrigby added awaiting review bug Categorizes issue or PR as related to a bug. labels May 16, 2024
@scottrigby scottrigby modified the milestones: 3.15.0, 3.15.1 May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm fails to retrieve a remote chart if there are local repos that are not cached
5 participants