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.

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

deprecate Versioned in favor of oci.Versioned #3887

Merged
merged 1 commit into from
Jul 18, 2024
Merged

deprecate Versioned in favor of oci.Versioned #3887

merged 1 commit into from
Jul 18, 2024

Conversation

thaJeztah
Copy link
Member

Update the Manifest types to use the oci implementation of the Versioned struct.

@thaJeztah thaJeztah marked this pull request as draft April 30, 2023 16:32
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03 ⚠️

Comparison is base (29b5e79) 56.63% compared to head (0fdf1b4) 56.60%.

📣 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    #3887      +/-   ##
==========================================
- Coverage   56.63%   56.60%   -0.03%     
==========================================
  Files         106      106              
  Lines       10674    10667       -7     
==========================================
- Hits         6045     6038       -7     
  Misses       3955     3955              
  Partials      674      674
Impacted Files Coverage Δ
manifest/schema1/manifest.go 33.82% <ø> (ø)
manifest/manifestlist/manifestlist.go 70.90% <100.00%> (-0.52%) ⬇️
manifest/ocischema/builder.go 64.70% <100.00%> (-1.34%) ⬇️
manifest/ocischema/manifest.go 74.19% <100.00%> (ø)
manifest/schema1/config_builder.go 71.35% <100.00%> (-0.31%) ⬇️
manifest/schema1/reference_builder.go 93.75% <100.00%> (-0.25%) ⬇️
manifest/schema2/builder.go 72.09% <100.00%> (+0.66%) ⬆️
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 Apr 30, 2023
Draft
2 tasks
@thaJeztah thaJeztah marked this pull request as ready for review April 30, 2023 16:55
@milosgajdos
Copy link
Member

If we are going to do this, we must look into #3869 as well

@thaJeztah thaJeztah force-pushed the deprecate_versioned branch 2 times, most recently from 2df0b42 to 2d6cefb Compare June 21, 2023 14:02
@davidspek
Copy link
Collaborator

@thaJeztah @milosgajdos What's the state of this PR now that #3869 has been merged? Is it still relevant? If so, it needs a rebase.

@thaJeztah
Copy link
Member Author

Did a quick rebase, but I kept the previous version in a backup branch, as I need to double-check I did the rebase right

@milosgajdos
Copy link
Member

This needs a rebase @thaJeztah

@github-actions github-actions bot added area/storage area/proxy Related to registry as a pull-through cache area/api labels Jul 16, 2024
@thaJeztah thaJeztah force-pushed the deprecate_versioned branch 2 times, most recently from 84e4f6e to 1deb80a Compare July 16, 2024 14:19
milosgajdos
milosgajdos approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

should there be a space here after // ?

No; there's a difference between machine-parseable comments and "actual" comments. Machine-parseable comments must follow a specific pattern;

  • don't start with a space
  • must start with a namespace (a-z)
  • followed by a colon (:)
  • and a comma-separated list of options

And (I think), must be their own paragraph when combined with regular comments (so a empty line before them); also see moby/moby#43804 which has some links.

Now, if only the gosec would follow the standards (instead of // #nosec) 😞

@milosgajdos
Copy link
Member

I see yeah, ignore me

https://golangci-lint.run/usage/false-positives/#nolint-directive

thaJeztah
thaJeztah commented Jul 17, 2024
View reviewed changes
manifest/manifestlist/manifestlist.go
Comment on lines -179 to +192
return m.MediaType, m.canonical, nil
var mediaType string
if m.MediaType == "" {
mediaType = v1.MediaTypeImageIndex
} else {
mediaType = m.MediaType
}

return mediaType, m.canonical, nil
Copy link
Member Author

Choose a reason for hiding this comment

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

It's been quite a while since I originally drafted this PR; I don't really recall making this part of the change 🙈. I think it's ok (i.e. default to the expected MediaType if none was set), but pointing it out just in case

@milosgajdos
Copy link
Member

This needs a rebase now @thaJeztah

@thaJeztah
deprecate Versioned in favor of oci.Versioned
1e89cf7 
Update the Manifest types to use the oci implementation of the Versioned
struct.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

rebased 👍

davidspek
davidspek approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@davidspek davidspek left a comment

Choose a reason for hiding this comment

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

This has been a long time coming. LGTM!

@milosgajdos milosgajdos merged commit 33b657b into distribution:main Jul 18, 2024
16 checks passed
@thaJeztah thaJeztah deleted the deprecate_versioned branch July 19, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milosgajdos milosgajdos milosgajdos approved these changes

@davidspek davidspek davidspek approved these changes

@corhere corhere Awaiting requested review from corhere

@Jamstah Jamstah Awaiting requested review from Jamstah

@wy65701436 wy65701436 Awaiting requested review from wy65701436

@squizzi squizzi Awaiting requested review from squizzi

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@thaJeztah @codecov-commenter @milosgajdos @davidspek