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 FetchReference() for Repository Target #124

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

Wwwsylvia
Copy link
Member

Closes #122

registry/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Show resolved Hide resolved
registry/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Show resolved Hide resolved
Copy link
Contributor

@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.

Does this have both casss of GET by digest and by tag name?

@@ -739,6 +796,58 @@ func (s *manifestStore) Resolve(ctx context.Context, reference string) (ocispec.
}, nil
}

// FetchTag fetches the content identified by a reference.
// It is equivalent to call `Resolve()` and then `Fetch()`
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be equivalent of resolve and fetch. They are 2 API requests where are this should be only one GET request.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is equivalent in terms of functionality. We need to mention that it is more efficient as it has only 1 GET request in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree but would like to keep the docs for the function without ambiguity. It is clearly 2 APIs and they have different behaviors depending on intermediaries.

Maybe edit to keep it concise and have more details in the example.
//FetchTag fetches the content identified by the reference. The reference may be a tag or digest.

Copy link
Contributor

Choose a reason for hiding this comment

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

On now thinking more - its sounds incorrect to call this method FetchTag if we are passing a digest.
We should consider some other name like FetchByReference(string reference....).
Or am I missing something here?

Copy link
Member Author

@Wwwsylvia Wwwsylvia Mar 29, 2022

Choose a reason for hiding this comment

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

Agreed. I've renamed it to FetchReference and made the doc concise.

@Wwwsylvia Wwwsylvia changed the title Add FetchTag() for Repository Target Add FetchReference() for Repository Target Mar 29, 2022
@sajayantony
Copy link
Contributor

The renames look really good. The fact that we are standardizing on ref seems much more consistent. Do you have a test case for this format

docker image pull k8s.gcr.io/pause-amd64:some-made-up-tag@sha256:59eec8837a4d942cc19a52b8c09ea75121acc38114a2c68b98983ce9356b8610

@Wwwsylvia
Copy link
Member Author

Wwwsylvia commented Apr 2, 2022

The renames look really good. The fact that we are standardizing on ref seems much more consistent. Do you have a test case for this format

docker image pull k8s.gcr.io/pause-amd64:some-made-up-tag@sha256:59eec8837a4d942cc19a52b8c09ea75121acc38114a2c68b98983ce9356b8610

@sajayantony Test cases added.

Signed-off-by: Lixia (Sylvia) Lei <lixia_lei@outlook.com>
Signed-off-by: Lixia (Sylvia) Lei <lixia_lei@outlook.com>
Signed-off-by: Sylvia Lei <lixia_lei@outlook.com>
Signed-off-by: Sylvia Lei <lixia_lei@outlook.com>
Signed-off-by: Sylvia Lei <lixia_lei@outlook.com>
@Wwwsylvia
Copy link
Member Author

@shizhMSFT @sajayantony Could you please review the updates?

registry/remote/repository.go Outdated Show resolved Hide resolved
registry/remote/repository.go Outdated Show resolved Hide resolved
Signed-off-by: Sylvia Lei <lixia_lei@outlook.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@sajayantony
Copy link
Contributor

@qweeah will you add an example for this one?

@sajayantony sajayantony merged commit 6c9a83f into oras-project:main Apr 5, 2022
@Wwwsylvia Wwwsylvia deleted the fetchtag branch September 19, 2022 11:03
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.

Add Function to Fetch Manifest Using Reference
3 participants