registry: deprecate APIEndpoint.TrimHostname #49005
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
18505e1 to
10212e3
Compare
Member
Author
|
Ahm, right, so this was a bit of a gamble; the field was not documented, and it was pretty unclear in which situations trimming was needed. Possibly "pull" and/or "auth" endpoints should not trim, and push endpoints should, but I need to dig a bit deeper. If we cannot remove this field, at least we should document it, and outline under what situations the field should be set (or not). For future reference; this test is currently failing (I need to look if the test just needs to be adjusted, or if the failure is legit); |
10212e3 to
bb5ec1f
Compare
This comment was marked as spam.
This comment was marked as spam.
1 similar comment
This comment was marked as spam.
This comment was marked as spam.
bb5ec1f to
a92342a
Compare
ee2938f to
2a63cc8
Compare
thaJeztah
commented
Dec 3, 2024
Comment on lines
-298
to
+302
| case r.Method == "GET" && r.URL.Path == "/v2": | ||
| w.WriteHeader(http.StatusOK) | ||
| case r.Method == "GET" && r.URL.Path == "/v2/docker.io/library/testremotename/blobs/"+expectedDigest.String(): | ||
| case r.Method == "GET" && r.URL.Path == "/v2/library/testremotename/blobs/"+expectedDigest.String(): |
Member
Author
There was a problem hiding this comment.
As far as I could tell, this docker.io/ in the path was purely to match the URLs we constructed in the test (which didn't set TrimHostpath, but in non-test code, it would be set?
Member
Author
|
Possibly flaky test; |
Member
Author
|
Discussed with @dmcgowan to get more clarity on the purpose of this field; This option was added for possible future expansion, allowing registry-mirrors to get the full reference of the image (including domain-name), for them to host a mirror for multiple upstreams on the same registry. That approach will unlikely be implemented, and containerd has a different approach for this, where the reference to the original registry is passed through a query parameter instead. From this, it looks indeed OK to remove this, although he mentioned Notary using some of this, so we need to double-check if anything in docker/cli would depend on it (and/or break when removing this). |
2a63cc8 to
67b79a8
Compare
This field was added in 19515a7, but looks to be always set for endpoints used, so we can trim remote names unconditionally. This option was added for possible future expansion, allowing registry- mirrors to get the full reference of the image (including domain-name), for them to host a mirror for multiple upstreams on the same registry. That approach will unlikely be implemented, and containerd has a different approach for this, where the reference to the original registry is passed through a query parameter instead. The field is unlikely used outside of our codebased, but deprecating it before removal just in case. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
67b79a8 to
3014d6d
Compare
vvoland
approved these changes
Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
registry: deprecate APIEndpoint.TrimHostname
This field was added in 19515a7, but looks
to be always set for endpoints used, so we can trim remote names unconditionally.
This option was added for possible future expansion, allowing registry-
mirrors to get the full reference of the image (including domain-name),
for them to host a mirror for multiple upstreams on the same registry.
That approach will unlikely be implemented, and containerd has a different
approach for this, where the reference to the original registry is passed
through a query parameter instead.
The field is unlikely used outside of our codebased, but deprecating it
before removal just in case.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)