2nd step toward Investigate and propose package repositories API with similar core interface to packages API. #3496 #4476
Conversation
|
Hi Greg. Thanks for the nice focused change again :) Note that the changes to the auto-generated files appear to be a bit excessive (in the dashboard) which probably means you need to run |
I did run 'yarn prettier' before pushing. Let me know which I will manually remove any dashboard-related changes from this PR. Thanks |
Hmm, could be actual changes then. I'll check locally later and let you know. |
| }, | ||
| statusCode: codes.OK, | ||
| }, | ||
| /* |
There was a problem hiding this comment.
Is this commented as a reminder TODO later, or did you mean to delete it? The test mock that you modified below could return a NotFound easily enough?
There was a problem hiding this comment.
whooops. Started and forgot about it :-) Will fix
| CustomDetail: nil, | ||
| Status: defaultRepoStatus, | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for doing the tests here following the existing pattern (using Make* factory methods etc.)
There was a problem hiding this comment.
my pleasure :-)
| // fix up a couple of fields that don't come from the chart tarball | ||
| repoUrl := repo.Spec.URL | ||
| if repoUrl == "" { | ||
| return nil, status.Errorf(codes.NotFound, "Missing required field spec.url on repository %q", repoName) |
There was a problem hiding this comment.
Hmmm, not sure about a not found here? With helm you can install a chart from a repo, then later delete the repo. Is that same situation impossible with flux? Or do we return a not found in helm?
There was a problem hiding this comment.
This was here a long time, I just moved the code around a bit. I think it was for some negative test I wrote. Anyway, repo.Spec.URL is a required field in flux https://fluxcd.io/docs/components/source/helmrepositories/ so in theory the if clause never triggers under normal circumstances
There was a problem hiding this comment.
regarding your question about flux. The helm chart CR has a back reference to the source. So I don't think this situation is possible. If you delete the helm repo, then associated helmcharts are deleted too. But I will verify now just to make sure
There was a problem hiding this comment.
I stand corrected. It is possible to delete the helmrepository. Then the helm chart goes into a 'bad' (not ready) state:
[v1alpha1]$ kubectl get helmchart
NAME CHART VERSION SOURCE KIND SOURCE NAME READY STATUS AGE
default-my-podinfo podinfo * HelmRepository podinfo True Pulled 'podinfo' chart with version '6.1.0'. 14s
[v1alpha1]$ kubectl get helmrepository
NAME URL READY STATUS AGE
podinfo https://stefanprodan.github.io/podinfo True Fetched revision: 4995e6e5e300c68f9ea8e0f26c91e42f0dbaf1be46e4e464b25988eb5ae86d4b 97s
[v1alpha1]$ kubectl delete helmrepository/podinfo
helmrepository.source.toolkit.fluxcd.io "podinfo" deleted
[v1alpha1]$ kubectl get helmchart
NAME CHART VERSION SOURCE KIND SOURCE NAME READY STATUS AGE
default-my-podinfo podinfo * HelmRepository podinfo True Pulled 'podinfo' chart with version '6.1.0'. 56s
[v1alpha1]$ kubectl get helmchart
NAME CHART VERSION SOURCE KIND SOURCE NAME READY STATUS AGE
default-my-podinfo podinfo * HelmRepository podinfo False failed to retrieve source: HelmRepository.source.toolkit.fluxcd.io "podinfo" not found 64s
[v1alpha1]$ kubectl get helmchart
NAME CHART VERSION SOURCE KIND SOURCE NAME READY STATUS AGE
default-my-podinfo podinfo * HelmRepository podinfo False failed to retrieve source: HelmRepository.source.toolkit.fluxcd.io "podinfo" not found 69s
When this happens flux plugin GetAvailablePackageDetail() will raise a NotFound error a lot earier than the line you pointed out. It'll be on line 52 (repo, err := s.getRepoInCluster(ctx, repoName)) call. If you think we should change the way it works, let me know
Also the corresponding HelmRelease goes into 'bad' state:
$ kubectl get helmrelease
NAME READY STATUS AGE
my-podinfo False HelmChart 'default/default-my-podinfo' is not ready 11m
In short, I would say deleting the repo is not recommended 😃
There was a problem hiding this comment.
Yeah, fine as is then. Thanks for checking.
added GetPackageRepositoryDetail() API as outlined in the spec.