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

feat: Support pulling arbitrary manifest config #480

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

lizMSFT
Copy link
Contributor

@lizMSFT lizMSFT commented Aug 4, 2022

#274 enables the CLI to pull manifest config. However, the user must provide the desired media type of the config or use the default. This PR allows pulling arbitrary manifest config.

If the user does not specify the desired media type, the code will get the 0th indexed node as the manifest config if its parent node is a manifest.
Note: For a manifest, the 0th indexed element is always a manifest config. ref: https://github.com/oras-project/oras-go/blob/6a09a65fcc0b96c3448e988c1727ed154c2388ea/content/graph.go#L58

Examples:

$ oras pull --manifest-config config.json localhost:5000/hello:latest
Downloaded  3ad08644ab5a artifact.txt
Downloaded  98df5c495df6 config.json
Pulled localhost:5000/hello:latest
Digest: sha256:8627dfe823698a566e105d961045eafcaa9115b6faf1f625edd85df558421bd5

Resolves: #275
Signed-off-by: Zoey Li zoeyli@microsoft.com

cmd/oras/pull.go Outdated Show resolved Hide resolved
Copy link
Contributor

@qweeah qweeah left a comment

Choose a reason for hiding this comment

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

Verified on my side and functionally looks good.

@qweeah
Copy link
Contributor

qweeah commented Aug 5, 2022

@shizhMSFT Do we need to print warning if: no config media type is specified and there is already a name(title annotation) in the config descriptor?

@sajayantony
Copy link
Contributor

Recommend simplifying this to to just oras pull --config. I don't believe there is any other object in OCI that is called config.

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2022

Codecov Report

Merging #480 (e098299) into main (7c6fbdb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #480   +/-   ##
=======================================
  Coverage   55.69%   55.69%           
=======================================
  Files           6        6           
  Lines         237      237           
=======================================
  Hits          132      132           
  Misses         90       90           
  Partials       15       15           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shizhMSFT
Copy link
Contributor

--config is already reserved for auth config.

See

  -c, --config stringArray       auth config path

@shizhMSFT
Copy link
Contributor

@lizMSFT @sajayantony @qweeah There is another option that we can rename --config to --auth-config and use --config for --manifest-config.

@qweeah
Copy link
Contributor

qweeah commented Aug 12, 2022

@lizMSFT @sajayantony @qweeah There is another option that we can rename --config to --auth-config and use --config for --manifest-config.

How about --cred-file? Previously we used --config to align with the naming of docker config but it's not easy to understand out of the docker context.

We can still use -c for short.

@sajayantony
Copy link
Contributor

This is tricky. If you have a config file that’s for auth and config file for manifest.
Im ok with manifest config or auth config. since —config /auth-config is a global flag and if it was there before then we might not want to break previous non alpha - releases.
Basically you folks decide. 😁

@lizMSFT
Copy link
Contributor Author

lizMSFT commented Aug 14, 2022

Yes, I am feeling the same. It would be better not change the name for now since it will bring breaking changes and it currently doesn't bring additional value. Besides, consumers will need to adjust accordingly, which might make them confused.

@lizMSFT
Copy link
Contributor Author

lizMSFT commented Aug 15, 2022

Create a new issue for rename --manifest-config: #495
cc @shizhMSFT @qweeah @sajayantony

cmd/oras/pull.go Outdated Show resolved Hide resolved
@shizhMSFT
Copy link
Contributor

@sajayantony Let's continue the discussion in #495.

@shizhMSFT
Copy link
Contributor

@shizhMSFT Do we need to print warning if: no config media type is specified and there is already a name(title annotation) in the config descriptor?

No, we don't.

Signed-off-by: Zoey Li <zoeyli@microsoft.com>
Signed-off-by: Zoey Li <zoeyli@microsoft.com>
Signed-off-by: Zoey Li <zoeyli@microsoft.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

@shizhMSFT shizhMSFT merged commit 3975738 into oras-project:main Aug 15, 2022
@shizhMSFT shizhMSFT linked an issue Aug 15, 2022 that may be closed by this pull request
TerryHowe pushed a commit to TerryHowe/oras that referenced this pull request Feb 2, 2023
Signed-off-by: Zoey Li <zoeyli@microsoft.com>
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.

Pull arbitrary manifest config How do I get the config via oras.Pull?
5 participants