-
Notifications
You must be signed in to change notification settings - Fork 176
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: add platform option to push command #1500
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1500 +/- ##
==========================================
- Coverage 85.97% 85.87% -0.10%
==========================================
Files 118 118
Lines 4228 4255 +27
==========================================
+ Hits 3635 3654 +19
- Misses 354 359 +5
- Partials 239 242 +3 ☔ View full report in Codecov by Sentry. |
641cbba
to
7e915b1
Compare
cmd/oras/root/push.go
Outdated
desc := ocispec.Descriptor{ | ||
MediaType: oras.MediaTypeUnknownConfig, | ||
Digest: digest.FromBytes(blob), | ||
Size: int64(len(blob)), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This part can be replaced by func NewDescriptorFromBytes(mediaType string, content []byte) ocispec.Descriptor
of oras-go. Reference: NewDescriptorFromBytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd/oras/root/push.go
Outdated
if opts.Platform.Platform != nil && opts.artifactType != "" { | ||
return errors.New("--artifact-type and --platform cannot both be provided for 1.0 OCI image") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can be used together, can't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed they can be used together. We can populate the artifactType
as mediaType
into the config descriptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cmd/oras/root/push.go
Outdated
return err | ||
} | ||
desc := ocispec.Descriptor{ | ||
MediaType: oras.MediaTypeUnknownConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value depends on the image-spec version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the image-spec
version is v1.0
. The media type can also be the value of --artifact-type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
3038b7b
to
fafd301
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit, but I am not a maintainer.
cmd/oras/root/push.go
Outdated
if opts.Flag == option.ImageSpecV1_0 { | ||
if opts.artifactType != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about
if opts.Flag == option.ImageSpecV1_0 { | |
if opts.artifactType != "" { | |
if opts.Flag == option.ImageSpecV1_0 && opts.artifactType != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
fafd301
to
a8a6a36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8d4062e
to
35fe5b7
Compare
76c1ad3
to
45456d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor suggestion
956c7e3
to
1334a8d
Compare
Sorry merging 55cf426 is not intended, I was playing with the PR locally and the upstream is accidentally set to this PR. |
55cf426
to
1334a8d
Compare
reverted |
c8d6849
to
e0ef359
Compare
Signed-off-by: Terry Howe <[email protected]>
e0ef359
to
4b2321a
Compare
What this PR does / why we need it:
Add the platform option to the push command for example:
Create the manifest for example:
The results
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Closes #1066
Partial #1053
Please check the following list: