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.

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

Add RC1 Support #12

Merged
merged 13 commits into from
May 19, 2022
Merged

Add RC1 Support #12

merged 13 commits into from
May 19, 2022

Conversation

akashsinghal
Copy link

@akashsinghal akashsinghal commented Apr 22, 2022

  • Updates artifact spec to RC1
  • Implements sorting
  • Implements filtering
  • Implements registry level OCI discover
  • Implements repository level OCI discover
  • Implements pagination
  • Adds tests for artifact manifest handler
  • Adds tests for referrers API

References #4

registry/extension/oras/referrers.go Outdated Show resolved Hide resolved
registry/extension/oras/referrers.go Outdated Show resolved Hide resolved
registry/extension/oras/referrers.go Outdated Show resolved Hide resolved
registry/extension/oras/artifactmanifesthandler.go Outdated Show resolved Hide resolved
registry/extension/oras/artifactmanifest.go Outdated Show resolved Hide resolved
@akashsinghal
Copy link
Author

@aviral26 thanks for the review! I have addressed your comments:

  • I added media type validations
  • It starts from page 1 by default if it cannot find any matching manifests provided in the nextToken
  • The nextToken takes in a comma separated list of digests. The Link header generated returns the last 3 referrer manifest digests in the nextToken field. (I have also set the minimum page size to 3 to accommodate 3 digests being returned. If n < 3 is provided, it defaults to the default page size of 50).
  • Added a new ErrorMalformedNextToken error. It is thrown if the nextToken parsing fails at the list and individual digest level

registry/extension/oras/artifactservice.go Outdated Show resolved Hide resolved
registry/extension/oras/artifactservice.go Show resolved Hide resolved
registry/extension/oras/artifactservice.go Outdated Show resolved Hide resolved
registry/extension/oras/referrers.go Outdated Show resolved Hide resolved
registry/extension/oras/referrers.go Show resolved Hide resolved
registry/extension/oras/artifactservice.go Outdated Show resolved Hide resolved
registry/extension/oras/referrers.go Outdated Show resolved Hide resolved

// ErrorCodeMalformedNextToken is returned when uploading a blob if the
// provided digest does not match the blob contents.
ErrorCodeMalformedNextToken = errcode.Register(errGroup, errcode.ErrorDescriptor{

Choose a reason for hiding this comment

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

I've been giving this some thought myself, but we may want to introduce some way to register errors from the extension API itself.

registry/extension/oci/discover.go Outdated Show resolved Hide resolved
registry/extension/oci/oci.go Outdated Show resolved Hide resolved
enumerateExtension.Endpoints = append(enumerateExtension.Endpoints, path)
}

enumeratedExtensions = append(enumeratedExtensions, enumerateExtension)
// add extension to list if endpoints exist
if len(enumerateExtension.Endpoints) > 0 {

Choose a reason for hiding this comment

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

Just looking for some context on the changes to this file.

  1. Previously, both repository and registry routes were returned but now it seems to be one of the two?
  2. Why are extensions added only if endpoints exist - when would that not be the case?

Copy link
Author

Choose a reason for hiding this comment

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

  1. Yes that's correct. So previously, both routes were returned but the subset of routes we want to return depends on if the discover request is at the registry or repository level. The distinction is now being made at the EnumerateRegistered level. If the Repository field in the context is nil, then we know that we are operating at the registry level and only want to return registry level routes for that extension namespace.
  2. It's possible that for an extension namespace, that there doesn't exist any routes at the scoped registry or repository level. In that case, we wouldn't want to add the enumerateExtension object to the list since no endpoints were added. The referrers extension would be an example: referrers only operates at the repository level but if we are enumerating at the registry level, then the scopedRoutes would be empty and we wouldn't want to add to the enumeratedExtensions list.

})
// append the descriptors, which don't have a created annotation, to the end
referrersSorted = append(referrersSorted, referrersUnsorted...)
return referrersSorted, nil

Choose a reason for hiding this comment

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

This now may also contain a whole lot of annotations. Do we want to limit the maximum size of the returned response?

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the annotations from the referrers response since the referrers spec sample response does not contain the field. There's also a concern that a large number of annotations can cause response size issues.

@akashsinghal
Copy link
Author

Follow up PR to add GC and more extensions unit tests will be made to the rc1 branch

@avtakkar avtakkar merged commit 0a64459 into oras-project:rc1 May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants