Skip to content

cli/trust: remove special handling for "plugin" Class - #4114

Merged
thaJeztah merged 1 commit into
docker:masterfrom
thaJeztah:remove_registry_service_step2
Mar 23, 2023
Merged

cli/trust: remove special handling for "plugin" Class#4114
thaJeztah merged 1 commit into
docker:masterfrom
thaJeztah:remove_registry_service_step2

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Mar 22, 2023

Copy link
Copy Markdown
Member

This code depended on the registry Service interface, which has been removed,
so needed to be refactored. Digging further into the reason this code existed,
it looked like the Class=plugin was previously required on Docker Hub to handle
plugins, but this requirement is no longer there, so we can remove this special
handling.

This patch removes the special handling to both remove the use of the registry.Service
interface, as well as removing complexity that is no longer needed.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Comment threadcli/trust/trust.go Outdated
Repository: repoInfo.Name.Name(),
Actions: actions,
Class: repoInfo.Class,
Class: repoInfo.Class, // TODO(thaJeztah): if Class is no longer needed for plugins, does that mean we can remove it as a whole?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

... and now I'm wondering if this Class is needed at all anymore (of there's only "image" or "").

I do see some handling in "distribution"; https://github.com/distribution/distribution/blob/e5d5810851d1f17a5070e9b6f940d8af98ea3c29/registry/client/auth/session.go#L154-L164

// String returns the string representation of the repository// using the scope grammarfunc (rsRepositoryScope) String() string {
repoType:="repository"// Keep existing format for image class to maintain backwards compatibility// with authorization servers which do not support the expanded grammar.ifrs.Class!=""&&rs.Class!="image" {
repoType=fmt.Sprintf("%s(%s)", repoType, rs.Class)
}
returnfmt.Sprintf("%s:%s:%s", repoType, rs.Repository, strings.Join(rs.Actions, ","))
}

And that looks to be "only if it's NOT "" or "image"

@codecov-commenter

codecov-commenter commented Mar 22, 2023

Copy link
Copy Markdown

Codecov Report

Merging #4114 (2dc9fb9) into master (bfe87fd) will increase coverage by 0.01%.
The diff coverage is 57.14%.

❗ Current head 2dc9fb9 differs from pull request most recent head 46095c6. Consider uploading reports for the commit 46095c6 to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## master #4114 +/- ##
==========================================
+ Coverage 59.14% 59.16% +0.01% 
==========================================
Files 287 287 Lines 24741 24717 -24 ==========================================
- Hits 14634 14623 -11 + Misses 9221 9210 -11 + Partials 886 884 -2 

@thaJeztah
thaJeztahforce-pushed the remove_registry_service_step2 branch from 2dc9fb9 to 46095c6CompareMarch 22, 2023 21:34
@thaJeztahthaJeztah mentioned this pull request Mar 22, 2023
@thaJeztahthaJeztah added this to the v-next milestone Mar 23, 2023
This code depended on the registry Service interface, which has been removed,
so needed to be refactored. Digging further into the reason this code existed,
it looked like the Class=plugin was previously required on Docker Hub to handle
plugins, but this requirement is no longer there, so we can remove this special
handling.
This patch removes the special handling to both remove the use of the registry.Service
interface, as well as removing complexity that is no longer needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the remove_registry_service_step2 branch from 46095c6 to 0ba820eCompareMarch 23, 2023 12:45
@thaJeztah
thaJeztah marked this pull request as ready for review March 23, 2023 12:45
@thaJeztahthaJeztah added area/plugins area/trust kind/refactor PR's that refactor, or clean-up code labels Mar 23, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah@codecov-commenter@corhere@cpuguy83