Flux oci support 9: fix helm repository cache out-of-sync when remote contents changes #5222
Conversation
✅ Deploy Preview for kubeapps-dev canceled.Built without sensitive environment variables
|
| func (s *repoEventSink) onAddOciRepo(repo sourcev1.HelmRepository) ([]byte, bool, error) { | ||
| log.Infof("+onAddOciRepo(%s)", common.PrettyPrint(repo)) | ||
| defer log.Info("-onAddOciRepo") | ||
|
|
||
| // I am disabling repo cache due to |
There was a problem hiding this comment.
Are you or is it just a leftover comment? It seems you are still using the cacheEntryValue, no?
There was a problem hiding this comment.
just left over comment. WIll fix
|
|
||
| if s.chartCache != nil { | ||
| fn := downloadOCIChartFn(ociChartRepo) | ||
| if err = s.chartCache.SyncCharts(charts, fn); err != nil { |
There was a problem hiding this comment.
I guess this is still syncing by just adding any new versions, no? I mean, in the event of a chart deprecation, the package won't get removed from the cache, no?
If so, wouldn't the upstream checksum always differ from the cached one which may lead to a permanent cache-miss state?
Not sure, just asking, though.
There was a problem hiding this comment.
I guess this is still syncing by just adding any new versions, no? I mean, in the event of a chart deprecation, the package won't get removed from the cache, no?
correct. BTW, related #4115 is still open. Just didn't get to it yet
If so, wouldn't the upstream checksum always differ from the cached one which may lead to a permanent cache-miss state?
Not sure, just asking, though.
I don't think so. The checksum is computed by my code without having to look what charts are in the local chart cache and is stored separately from the cached charts. It only looks at the charts and corresponding tags on the remote. If different from what is store in the cache, the new checksum is stored in the entry again
There was a problem hiding this comment.
Ah, ok, ok, got it. If we aren't looking at the charts in the local cache for computing it, then it doesn't seem an issue. Thanks for the clarification!
| @@ -59,7 +59,7 @@ function pushChartToMyGitHubRegistry() { | |||
| -H "Accept: application/vnd.github+json" \ | |||
| /user/packages/container/helm-charts%2Fpodinfo/versions | jq -rc '.[].metadata.container.tags[]') | |||
| echo | |||
| echo Remote Repository aka Package [${L_YELLOW}$GITHUB_OCI_REGISTRY_URL/podinfo${NC}] / All Versions | |||
| echo -e Remote Repository aka Package [${L_YELLOW}$GITHUB_OCI_REGISTRY_URL/podinfo${NC}] / All Versions | |||
There was a problem hiding this comment.
Nitpick: echo -e is not POSIX-compliant (although we are using it a couple of times in the CI and it works). Just for the record, I'm ok with it, thoguh.
There was a problem hiding this comment.
fair enough. So be it. The benefits of using it at the moment outweigh this drawback. Makes output so much easier to read by focusing on the keywords that are colored different that the rest of the output. If this becomes an issue, I'll get rid of it
|
The CI fix is now out, once you merge latest |
Thank you for the review |
see #5212 (comment) for description of the problem and the fix
Also renamed cache funcs for simplicity and to save time on typing:
All tests (integration+unit) passing