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

Refactor pull command #395

Closed
wants to merge 57 commits into from
Closed

Refactor pull command #395

wants to merge 57 commits into from

Conversation

qweeah
Copy link
Contributor

@qweeah qweeah commented May 23, 2022

This PR upgrades pull command towards oras-go v2.

Resolves #325

qweeah added 25 commits May 16, 2022 22:48
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@qweeah qweeah changed the title Refactor push command Refactor pull command May 23, 2022
@qweeah qweeah marked this pull request as draft May 23, 2022 03:47
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@qweeah qweeah requested a review from shizhMSFT May 30, 2022 06:04
cmd/oras/internal/option/remote.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/remote.go Outdated Show resolved Hide resolved
cmd/oras/pull.go Outdated Show resolved Hide resolved
cmd/oras/pull.go Outdated Show resolved Hide resolved
internal/status/pull.go Outdated Show resolved Hide resolved
internal/status/push.go Show resolved Hide resolved
internal/status/push.go Outdated Show resolved Hide resolved
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@qweeah qweeah requested a review from shizhMSFT June 2, 2022 02:57
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@qweeah
Copy link
Contributor Author

qweeah commented Jun 6, 2022

@shizhMSFT I have refactored the pull caching. It borrows the cas proxy implementation in oras-go, can we just make that package public?

@shizhMSFT
Copy link
Contributor

@shizhMSFT I have refactored the pull caching. It borrows the cas proxy implementation in oras-go, can we just make that package public?

/cc @Wwwsylvia for inputs

internal/status/push.go Show resolved Hide resolved
internal/status/pull.go Show resolved Hide resolved
// option, see https://github.com/oras-project/oras-go/issues/59.
func (t *PullTracker) Push(ctx context.Context, expected ocispec.Descriptor, content io.Reader) error {
option := t.ManifestConfigOption
if option != nil && option.MediaType == expected.MediaType && option.Name != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we check media type here? What if the media type does not match?

Copy link
Contributor Author

@qweeah qweeah Jun 7, 2022

Choose a reason for hiding this comment

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

Background: The manifest config option is set through the --manifest-config flag, via which user can specify a file name and a media type for target config to be downloaded. If the media type is not provided, application/vnd.unknown.config.v1+json will be used.

So here we should only rewrite the file name when a blob media type matches the target config media type. If the media type doesn't match, then current blob is not the target config blob.

Copy link
Member

Choose a reason for hiding this comment

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

OK interesting. Maybe we can add some comments here?

cmd/oras/pull.go Outdated

// Copy Options
ctx, _ := opts.SetLoggerLevel()
var dstStore = file.New(opts.Output)
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not dstStore := file.New(opts.Output)?

@Wwwsylvia
Copy link
Member

@shizhMSFT I have refactored the pull caching. It borrows the cas proxy implementation in oras-go, can we just make that package public?

/cc @Wwwsylvia for inputs

Agreed. Since it's common for clients to implement caching, I think we can promote the proxy.go to the content package.

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@sajayantony
Copy link
Contributor

sajayantony commented Jun 7, 2022

public

@shizhMSFT I have refactored the pull caching. It borrows the cas proxy implementation in oras-go, can we just make that package public?

Are we planning to support it and do due diligence?

@qweeah
Copy link
Contributor Author

qweeah commented Jun 7, 2022

public

@shizhMSFT I have refactored the pull caching. It borrows the cas proxy implementation in oras-go, can we just make that package public?

Are we planning to support it and do die Diligence?

The biggest concern is that the current implementation of the proxy is only a read-through cache and what's deleted in the source won't reflected in the cache.

I have added a new issue oras-project/oras-go#163 for it, we can move the discussion to the issue since the change is related to oras-go.

@qweeah
Copy link
Contributor Author

qweeah commented Jun 14, 2022

@qweeah qweeah closed this Jun 14, 2022
@qweeah qweeah reopened this Jun 14, 2022
@qweeah
Copy link
Contributor Author

qweeah commented Jun 15, 2022

Closing since the copy options are supported in oras-go and no need to hack status tracking.

@qweeah qweeah closed this Jun 15, 2022
@qweeah qweeah deleted the pull-v2 branch August 25, 2023 03:07
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.

Error: not_found downloading the manifest-config from a remote artifact
4 participants