Skip to content

Conversation

@banjoh
Copy link
Contributor

@banjoh banjoh commented Dec 3, 2025

What this PR does / why we need it:

OCI references with tag+digest (e.g., chart:1.0@sha256:abc...) failed with "invalid byte" error because the sha256: prefix was passed to hex.DecodeString().

Closed: #31600

Special notes for your reviewer:

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2025
@banjoh banjoh changed the title fix: strip digest algorithm prefix from digest string fix: handle OCI digest algorithm prefix in chart downloader Dec 3, 2025
OCI references with tag+digest (e.g., chart:1.0@sha256:abc...) failed with "invalid byte" error because the sha256: prefix was passed to hex.DecodeString().

Signed-off-by: Evans Mungai <mbuevans@gmail.com>
@banjoh banjoh force-pushed the em/fix-digest-tag-inconsistency branch from a8697d7 to c5112b9 Compare December 3, 2025 13:52
Signed-off-by: Evans Mungai <mbuevans@gmail.com>
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 3, 2025
@banjoh banjoh added the bug Categorizes issue or PR as related to a bug. label Dec 3, 2025
TestUsername string
TestPassword string
Client *ociRegistry.Client
ManifestDigest string // Digest of the pushed oci-dependent-chart manifest
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit odd to put ManifestDigest here in a object about server configuration. There's no other way to validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I'll see how best to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TerryHowe I addressed this

@TerryHowe
Copy link
Contributor

Thanks for pulling to PR together so fast @banjoh !

Signed-off-by: Evans Mungai <mbuevans@gmail.com>
Copy link
Contributor

@TerryHowe TerryHowe left a comment

Choose a reason for hiding this comment

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

/lgtm

@TerryHowe TerryHowe added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Dec 13, 2025
Comment on lines +520 to +522
if result != tt.expected {
t.Errorf("stripDigestAlgorithm(%q) = %q, want %q", tt.input, result, tt.expected)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if result != tt.expected {
t.Errorf("stripDigestAlgorithm(%q) = %q, want %q", tt.input, result, tt.expected)
}
assert.Equal(t, tt.expected, result)

Use testify?

}

func TestStripDigestAlgorithm(t *testing.T) {
tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

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

use a map here? e.g.

    tests := map[string]struct {
		input    string
		expected string
	}{
         "sha256 prefixed digest": {
             input:  "...

Then below:

for name, tt := range tests {
		t.Run(name, func(t *testing.T) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Categorizes issue or PR as related to a bug. Has One Approval This PR has one approval. It still needs a second approval to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent digest + tag handling in helm 4 install with OCI Chart

3 participants