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

Catch OCI version errors and return them #2311

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Catch OCI version errors and return them #2311

merged 2 commits into from
Apr 10, 2024

Conversation

manno
Copy link
Member

@manno manno commented Apr 9, 2024

Refers to #1606

The module we use would panic if version wasn't set or invalid.

With this change, an explicit version is not required for OCI charts. The module will reach out to the registry and fetch the tags and will resolve them to a version.

		// If empty, try to get the highest available tag
		// If exact version, try to find it
		// If semver constraint string, try to find a match

https://github.com/helm/helm/blob/release-3.14/pkg/downloader/chart_downloader.go#L144-L175

@manno manno force-pushed the catch-oci-url-errors branch from c30b5bd to 44ee701 Compare April 9, 2024 11:25
@manno manno force-pushed the catch-oci-url-errors branch from 44ee701 to ca7b941 Compare April 9, 2024 11:27
@manno manno marked this pull request as ready for review April 9, 2024 11:55
@manno manno requested a review from a team as a code owner April 9, 2024 11:55
result []string
expectedErr string
}{
// Note: These tests rely on external hosts and DNS resolution.
Copy link
Member Author

@manno manno Apr 10, 2024

Choose a reason for hiding this comment

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

Alternatively I can change the code, so I can mock the registry client. I'd then delete these test, because they mostly test the helm code. I would then add tests to check if the client is initialized and if error wrapping works.

Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

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

As discussed offline, despite the DNS and OCI registry dependencies, LGTM. As noted in the test driver, this nicely documents a few use cases.

@manno manno merged commit 3fe378f into main Apr 10, 2024
8 checks passed
@manno manno deleted the catch-oci-url-errors branch April 10, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants