8000 Make Descriptor an alias for oci.Descriptor by thaJeztah · Pull Request #3888 · distribution/distribution · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make Descriptor an alias for oci.Descriptor #3888

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2024
Merged

Make Descriptor an alias for oci.Descriptor #3888

merged 1 commit into from
Oct 7, 2024

Conversation

thaJeztah
Copy link
Member
@thaJeztah thaJeztah commented Apr 30, 2023 •
edited
Loading

With the removal of the Describable interface from this type, and
deprecation of the Versioned type, the Descriptor is now an exact
equivalent of the oci.Descriptor.

This patch makes Descriptor an alias for oci.Descriptor.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.08 ⚠️

Comparison is base (29b5e79) 56.63% compared to head (6025946) 56.55%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3888      +/-   ##
==========================================
- Coverage   56.63%   56.55%   -0.08%     
==========================================
  Files         106      106              
  Lines       10674    10689      +15     
==========================================
  Hits         6045     6045              
- Misses       3955     3967      +12     
- Partials      674      677       +3
Impacted Files Coverage Δ
manifest/schema1/manifest.go 33.82% <ø> (ø)
manifest/schema1/config_builder.go 69.63% <50.00%> (-2.03%) ⬇️
manifest/ocischema/builder.go 61.01% <58.33%> (-5.03%) ⬇️
manifest/schema2/builder.go 66.66% <58.33%> (-4.77%) ⬇️
manifest/manifestlist/manifestlist.go 70.90% <100.00%> (-0.52%) ⬇️
manifest/ocischema/manifest.go 74.19% <100.00%> (ø)
manifest/schema1/reference_builder.go 93.75% <100.00%> (-0.25%) ⬇️
manifest/schema2/manifest.go 80.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thaJeztah thaJeztah mentioned this pull request May 2, 2023
Merged
@thaJeztah thaJeztah force-pushed the replace_types_for_oci branch from 6025946 to e04a320 Compare July 19, 2024 13:53
@milosgajdos
Copy link
Member

This should be ok to work on now since the dependant PRs have been merged @thaJeztah 😄

@thaJeztah thaJeztah force-pushed the replace_types_for_oci branch 2 times, most recently from db42d58 to 8f58718 Compare September 18, 2024 17:44
@thaJeztah
Copy link
Member Author

One failure;

=== RUN   TestProxyManifestsMetrics
    proxymanifeststore_test.go:308: Expected manifestMetrics.BytesPulled 257 but got 274
    proxymanifeststore_test.go:311: Expected manifestMetrics.BytesPushed 257 but got 274
    proxymanifeststore_test.go:330: Expected manifestMetrics.BytesPulled 257 but got 274
    proxymanifeststore_test.go:333: Expected manifestMetrics.BytesPushed 514 but got 548
--- FAIL: TestProxyManifestsMetrics (0.28s)

I went looking where that originated from, and looks to be because the old type had an omitempty; for the Size field;

distribution/blobs.go

Lines 70 to 71 in 2314320

// Size in bytes of content.
Size int64 `json:"size,omitempty"`

Whereas the OCI type does not

distribution/vendor/github.com/opencontainers/image-spec/specs-go/v1/descriptor.go

Lines 29 to 30 in 2314320

// Size specifies the size in bytes of the blob.
Size int64 `json:"size"`

The test creates a manifest;

distribution/registry/proxy/proxymanifeststore_test.go

Lines 156 to 163 in 2314320

m := schema2.Manifest{
Versioned: specs.Versioned{SchemaVersion: 2},
MediaType: schema2.MediaTypeManifest,
Config: distribution.Descriptor{
MediaType: "foo/bar",
Digest: configDigest,
},
}

Before this PR that would produce (serialised);

{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json","config":{"mediaType":"foo/bar","digest":"sha256:96cc58f39ca8bde60ed58ccd0c72538939622537ec53181019f72360ccd0a0c8"},"layers":null}

After it produces;

{"schemaVersion":2,"mediaType":"application/vnd.docker.distribution.manifest.v2+json","config":{"mediaType":"foo/bar","digest":"sha256:96cc58f39ca8bde60ed58ccd0c72538939622537ec53181019f72360ccd0a0c8","size":0},"layers":null}

The extra ,"size":0 is 9 characters, which makes up for the difference.

@thaJeztah
Copy link
Member Author

The extra ,"size":0 is 9 characters, which makes up for the difference.

Ugh. well. LOL no the different is not 9 characters, but 17 😂 - Let me dig further

@thaJeztah
Copy link
Member Author

Ah! LOL, well, it helps if you use the right JSON output; this is the one I was looking for;

distribution/registry/proxy/proxymanifeststore_test.go

Line 180 in 2314320

sm, err := schema2.FromStruct(m)

That's a DeserializedManifest, and running sm.MarshalJSON() on that gives me indeed the difference;

Before: (length: 257)

{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
   "config": {
      "mediaType": "foo/bar",
      "digest": "sha256:96cc58f39ca8bde60ed58ccd0c72538939622537ec53181019f72360ccd0a0c8"
   },
   "layers": null
}

After: (length: 274)

{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
   "config": {
      "mediaType": "foo/bar",
      "digest": "sha256:96cc58f39ca8bde60ed58ccd0c72538939622537ec53181019f72360ccd0a0c8",
      "size": 0
   },
   "layers": null
}

@thaJeztah thaJeztah force-pushed the replace_types_for_oci branch from 8f58718 to d23f24f Compare September 18, 2024 20:12
@github-actions github-actions bot added the area/proxy Related to registry as a pull-through cache label Sep 18, 2024
@thaJeztah thaJeztah force-pushed the replace_types_for_oci branch from d23f24f to 3b4cc0c Compare September 18, 2024 20:29
@thaJeztah thaJeztah mentioned this pull request Sep 18, 2024
Merged
@thaJeztah
Copy link
Member Author

All green now, but moved the first commit to a separate PR;

@milosgajdos
Copy link
Member

Ok #4467 has now been merged. Might wanna open this now I suppose.

@thaJeztah
Make Descriptor an alias for oci.Descriptor
92b483e 
With the removal of the Describable interface from this type, and
deprecation of the Versioned type, the Descriptor is now an exact
equivalent of the oci.Descriptor.

This patch makes Descriptor an alias for oci.Descriptor.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the replace_types_for_oci branch from 3b4cc0c to 92b483e Compare October 3, 2024 18:00
@thaJeztah thaJeztah marked this pull request as ready for review October 3, 2024 18:00
@thaJeztah
Copy link
Member Author

Rebased! We should probably look carefully if the omitempty (therefore different length of digest) is something that could cause issues, or if that's OK to ignore within context of these changes.

@milosgajdos
Copy link
Member

Good idea, yeah. We have integration tests set up for S3 driver at least so if the tests pass that should give us confidence this change is 👌

Jamstah
Jamstah approved these changes Oct 7, 2024
View reviewed changes
Copy link
Collaborator
@Jamstah Jamstah left a comment

Choose a reason for hiding this comment

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

Size is required in the spec, so as long as the tests pass I think this is good.

@milosgajdos milosgajdos merged commit 740b311 into distribution:main Oct 7, 2024
16 checks passed
@thaJeztah thaJeztah deleted the replace_types_for_oci branch October 7, 2024 11:03
@thaJeztah thaJeztah mentioned this pull request Oct 7, 2024
Merged
@thaJeztah thaJeztah mentioned this pull request Oct 31, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jamstah Jamstah Jamstah approved these changes

@milosgajdos milosgajdos milosgajdos approved these changes

@squizzi squizzi Awaiting requested review from squizzi

@davidspek davidspek Awaiting requested review from davidspek

Assignees
No one assigned
Labels
area/proxy Related to registry as a pull-through cache impact/changelog status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thaJeztah @codecov-commenter @milosgajdos @Jamstah
0