Skip to content

Conversation

souleb
Copy link
Member

@souleb souleb commented Jun 5, 2022

This fixes a regression introduce in #690. We no longer cache Helm repository indexes when cache is enabled because of a defer function in the remote builder which unload the index.

If implemented this make sure we don't unload an index before caching it during a chart build phase. The index is unloaded downstream after successfully caching it.

Signed-off-by: Soule BA [email protected]

@souleb souleb requested a review from hiddeco June 5, 2022 21:36
@souleb souleb added bug Something isn't working area/helm Helm related issues and pull requests labels Jun 5, 2022
@souleb souleb requested a review from stefanprodan June 5, 2022 21:47
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @souleb

err = fmt.Errorf("could not load repository index for remote chart reference: %w", err)
return nil, &BuildError{Reason: ErrChartPull, Err: err}
}
defer remote.Unload()
Copy link
Contributor

Choose a reason for hiding this comment

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

With the removal of Unload(), my recommendation in https://github.com/fluxcd/source-controller/pull/690/files#r878152484 becomes more feasible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 👍 I am taking your recommendation into account in the coming PR that enable dependencies from OCI Helm Repositories.

If implemented this make sure we don't unload an index before caching it
during a chart build phase.

Signed-off-by: Soule BA <[email protected]>
@souleb souleb force-pushed the fix-cache-regression branch from 6581b7a to 0d4d78f Compare June 6, 2022 11:15
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks.

@darkowlzz darkowlzz merged commit b66ff92 into fluxcd:main Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants