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.

Sign up for GitHub

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

Jump to bottom

fix: OCI store resolve returns full descriptor #468

Merged
merged 1 commit into from
Mar 23, 2023
Merged

fix: OCI store resolve returns full descriptor #468

merged 1 commit into from
Mar 23, 2023

Conversation

oanatmaria
Copy link
Contributor

@oanatmaria oanatmaria commented Mar 21, 2023 •
edited
Loading

Based on discussion on #457, this PR returns a full descriptor on Resolve when the method it's called by tag and it returns a plain descriptor when the method it's called by digest.

@oanatmaria oanatmaria closed this Mar 21, 2023
@oanatmaria oanatmaria reopened this Mar 21, 2023
@oanatmaria oanatmaria changed the title OCI store resolve returns full descriptor fix: OCI store resolve returns full descriptor Mar 21, 2023
@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch from 23fc677 to 506896b Compare March 21, 2023 10:29
shizhMSFT
shizhMSFT approved these changes Mar 22, 2023
View reviewed changes
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

Wwwsylvia
Wwwsylvia reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@Wwwsylvia Wwwsylvia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @oanatmaria , thanks for the contribution! Do you want to update the same for ReadOnlyStore as well?

content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch from 506896b to c2c37cb Compare March 22, 2023 10:09
@oanatmaria
Copy link
Contributor Author

Hi @oanatmaria , thanks for the contribution! Do you want to update the same for ReadOnlyStore as well?

Hi @Wwwsylvia,

Thank you for the review. I've made the change for the readonlystore as well.

@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch from c2c37cb to 489b3a0 Compare March 22, 2023 10:19
Wwwsylvia
Wwwsylvia reviewed Mar 23, 2023
View reviewed changes
content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch 2 times, most recently from b8f7bfd to ddfa64a Compare March 23, 2023 12:08
@Wwwsylvia
Copy link
Member

Also wondering if we should call out the behavior difference between resolving by tag and resolving by digest in the doc comment of the Resolve() function? @oanatmaria What do you think?

@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch from ddfa64a to 087936c Compare March 23, 2023 12:17
@oanatmaria
Copy link
Contributor Author

Also wondering if we should call out the behavior difference between resolving by tag and resolving by digest in the doc comment of the Resolve() function? @oanatmaria What do you think?

I've updated the comments. Please let me know if they are correctly formulated.

Wwwsylvia
Wwwsylvia approved these changes Mar 23, 2023
View reviewed changes
Copy link
Member

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

LGTM

content/oci/oci.go Outdated Show resolved Hide resolved
OCI store resolve returns full descriptor
3239ae6 
Signed-off-by: oanatmaria <oana@aserto.com>
@oanatmaria oanatmaria force-pushed the resolve_returns_full_descriptor branch from 087936c to 3239ae6 Compare March 23, 2023 14:20
@Wwwsylvia Wwwsylvia merged commit e8225cb into oras-project:main Mar 23, 2023
@Wwwsylvia
Copy link
Member

Merged. Thank you! @oanatmaria

@shizhMSFT shizhMSFT mentioned this pull request May 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@Wwwsylvia Wwwsylvia Wwwsylvia approved these changes

@sajayantony sajayantony Awaiting requested review from sajayantony sajayantony is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker SteveLasker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oanatmaria @Wwwsylvia @shizhMSFT