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 Helm chart OCI registry handling #356

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

dbw7
Copy link
Contributor

@dbw7 dbw7 commented Apr 5, 2024

Closes #355 .

@dbw7 dbw7 requested review from jdob and atanasdinov April 5, 2024 17:16
@dbw7 dbw7 changed the title Fix registry validation Fix Helm chart OCI registry handling Apr 5, 2024
@@ -197,6 +197,10 @@ func TestDownloadChart_FailedRegistryLogin(t *testing.T) {
helmChart := &image.HelmChart{}
helmRepo := &image.HelmRepository{
URL: "oci://registry-1.docker.io/bitnamicharts/apache",
Authentication: image.HelmAuthentication{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test case without credentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say this is covered in TestDownloadChart as it uses an OCI repository with no credentials while all of the other failure cases of downloadChart are covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really. In fact we need to split up the cases where we:

  • return nil if there are credentials for successful operation
  • return error if there are credentials for failed operation
  • panic if there aren't credentials since the function should then never be called

@dbw7 dbw7 requested a review from atanasdinov April 8, 2024 15:59
@dbw7 dbw7 merged commit 17faffd into suse-edge:main Apr 8, 2024
2 checks passed
e-minguez pushed a commit to e-minguez/edge-image-builder that referenced this pull request Apr 9, 2024
* fix handling for oci registries

* update release notes and test

* add valid registry login case
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.

Helm fails getting charts stored in unauthenticated OCI registries
2 participants