Skip to content
This repository has been archived by the owner on Sep 23, 2022. It is now read-only.

Adding artifactType filtering to the Referrers API #74

Merged

Conversation

michaelb990
Copy link
Contributor

@michaelb990 michaelb990 commented Jul 26, 2022

Draft PR on top of #59 to add artifactType filtering in the Referrers API.

Pros:

  • Enables registries and clients to more easily work with large numbers of references for an image.
  • Prevents a use case which pushes large numbers of artifacts from impacting the performance of latency sensitive use cases.

Cons:

  • Fragments registry behavior because not all registries may end up supporting this, so clients will still need a fall back.
  • Only allows for filtering based on artifactType, not other fields or annotations.

Signed-off-by: Michael Brown <brownxmi@amazon.com>
docs/proposals/PROPOSAL_E.md Outdated Show resolved Hide resolved
docs/proposals/PROPOSAL_E.md Show resolved Hide resolved
@jdolitsky
Copy link
Member

Please rebase as #59 is in

@@ -179,6 +230,7 @@ For registries that do not support the `referrers` API, a tag MUST be pushed for
- `<ref>`: the digest from the `refers` field (limit of 64 characters)
- `<hash>`: the digest of this artifact (limit of 16 characters)
- `<type>`: type of artifact for filtering (limit of 5 characters)
- The type field is based on a mapping from `artifactType` to a short string for the tag.
Copy link
Member

Choose a reason for hiding this comment

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

this is probably to go away depending on #72

Comment on lines 205 to 207
"annotations": {
"artifactType": "application/vnd.example.icecream.v1"
}
Copy link
Member

Choose a reason for hiding this comment

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

should this be codified into the proposal?

Copy link
Member

Choose a reason for hiding this comment

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

So from the discussion just an indication of the field without he values makes the response for unmatched artifacts cachable.

@sajayantony
Copy link
Member

As a part of the response are all annotations expected to be returned in the referrers /cc @michaelb990

jdolitsky and others added 3 commits August 1, 2022 14:16
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
Co-authored-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
Co-authored-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Josh Dolitsky <393494+jdolitsky@users.noreply.github.com>
@jdolitsky
Copy link
Member

I've gone ahead and resolved merge conflicts / committed suggested changes to keep this moving. @michaelb990 - is this ready for review/merge?

@sudo-bmitch
Copy link
Contributor

I've gone ahead and resolved merge conflicts / committed suggested changes to keep this moving. @michaelb990 - is this ready for review/merge?

On the WG meeting last week, I think we ended up unsure if this made sense to implement since it would result in added complexity for registry servers that would reduce the ability to cache the output, and clients already have client-side filtering implemented to support the tag output and any registries that don't support filtering. Lets save this for a merge or close decision on tomorrow's meeting.

@michaelb990 michaelb990 marked this pull request as ready for review August 4, 2022 17:13
…lied

Signed-off-by: Michael Brown <brownxmi@amazon.com>
@michaelb990
Copy link
Contributor Author

@sudo-bmitch

On the WG meeting last week, I think we ended up unsure if this made sense to implement since it would result in added complexity for registry servers that would reduce the ability to cache the output, and clients already have client-side filtering implemented to support the tag output and any registries that don't support filtering. Lets save this for a merge or close decision on tomorrow's meeting.

  • Does the fact that it is a SHOULD not a MUST help with the complexity worries?
  • The recent updates to the annotations should solve the caching issues.
  • I think not having server-side filtering is one of the big drawbacks of the tagging scheme, so I'm hoping we see things move to server-side solutions "soon".

Will post to a vote.

Copy link
Member

@sajayantony sajayantony left a comment

Choose a reason for hiding this comment

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

LGTM for merging once the vote is in. Any minor adjustments can be made post merge in my opinion.

@sudo-bmitch
Copy link
Contributor

  • Does the fact that it is a SHOULD not a MUST help with the complexity worries?

I'd lean towards MAY, but that's not a strong opinion.

  • The recent updates to the annotations should solve the caching issues.

Thank you!

  • I think not having server-side filtering is one of the big drawbacks of the tagging scheme, so I'm hoping we see things move to server-side solutions "soon".

Will post to a vote.

The big advantage of having the API to me isn't filtering, but the lack of client side maintenance of the tag, and elimination of race conditions. (Plus it declutters the tag listing.) Thanks for getting the vote out. 👍

@dlorenc
Copy link

dlorenc commented Aug 4, 2022

+1 here too. Thanks!

@rchincha
Copy link

rchincha commented Aug 5, 2022

lgtm

@sudo-bmitch sudo-bmitch merged commit 818f114 into opencontainers:main Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants