specs-go/v1: unify the descriptor type #620
Conversation
|
Would it make sense to make this a well-known annotation instead? Seems like a good use for the field. Would require moving the annotations type to |
| OSFeatures []string `json:"os.features,omitempty"` | ||
|
|
||
| // Variant is an optional field specifying a variant of the CPU, for | ||
| // example `ppc64le` to specify a little-endian version of a PowerPC CPU. |
There was a problem hiding this comment.
This was copy pasted. If you want to change it, please do so in a follow up.
There was a problem hiding this comment.
@AkihiroSuda What was the PR that updates this? This all gets moved from index.go to descriptor.go, so we need to make sure it gets rebased appropriately.
|
Reposted from an emailed comment, which GitHub seems to have eaten: On Mon, Mar 20, 2017 at 07:01:12PM -0700, Erik Hollensbe wrote:
+1 to this. With platform-ness being just one way to select a given child, and the existing annotation org.opencontainers.image.ref.name being another parallel/complimentary way, I think moving the platform information into annotations makes sense. @stevvooe, you'd seemed to be on board with platform annotations earlier. Has something changed? We can always continue to inject the old platform properties if we want backwards compatibility with existing Docker manifest-list engines, although we may not be able to only use annotations if we require forward compatibility for existing Docker manifest-list blobs.
There's some background on string vs. |
@wking GitHub probably ate this because you are supposed to be blocked. Please do not comment on my pull requests. I have asked you, kindly, more than once.
Ref name isn't really binding in the way that platform is. Platform is just pulled up from the underlying config and can be validated. Ref name is a hint that shouldn't be trusted and can be ignored. This PR brings them to the same level so they can be used in filtration logic, but that doesn't point to their equivalence.
I don't see any reason to break from the structured approach. Having such a large change before 1.0 would be detrimental to the release and not provide value. Furthermore, annotations are not a dumping ground for stuff; they are meant for non-binding metadata. If we can provide structure for something, we absolutely should. Note that this PR doesn't change the context under which the platform field can be used. It only unifies the Go type. From the perspective of the specification, the |
|
Fair enough.
I hope my query wasn't a bother! I just thought it was a reasonable
question.
…-Erik
On Tue, Mar 21, 2017 at 12:32 PM Stephen Day ***@***.***> wrote:
Reposted from an emailed comment, which GitHub seems to have eaten
@wking <https://github.com/wking> GitHub probably ate this because you
are supposed to be blocked. Please do not comment on my pull requests. I
have asked you, kindly, more than once.
With platform-ness being just one way to select a given child, and the
existing annotation org.opencontainers.image.ref.name being another
parallel/complimentary way, I think moving the platform information into
annotations makes sense.
Ref name isn't really binding in the way that platform is. Platform is
just pulled up from the underlying config and can be validated. Ref name is
a hint that shouldn't be trusted and can be ignored. This PR brings them to
the same level so they can be used in filtration logic, but that doesn't
point to their equivalence.
Would it make sense to make this a well-known annotation instead? Seems
like a good use for the field. Would require moving the annotations type to
map[string]interface{} of course though, which might have consequences I
haven't foreseen.
I don't see any reason to break from the structured approach. Having such
a large change before 1.0 would be detrimental to the release and not
provide value. Furthermore, annotations are not a dumping ground for stuff;
they are meant for non-binding metadata. If we can provide structure for
something, we absolutely should.
Note that this PR doesn't change the context under which the platform
field can be used. It only unifies the Go type. From the perspective of the
specification, the Platform field can only be used with indexes. Moving
platform to annotations or allowing platform descriptions outside of this
context are outside the scope of this PR. This PR is only present to make
the descriptor type easier to work with.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#620 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABJ6-k6uodJdG6lJ89qq7e8dr2g49XLks5roCW-gaJpZM4MjIiz>
.
|
No, it is a completely reasonable question. I hope the clarification was helpful. |
|
I guess we need another PR to update specification accordingly to make it consistent with the changes in this PR. |
|
LGTM |
|
If such changes, calls to descriptor will be no errors in the |
Unifying the descriptor type makes it much easier to handler descriptors in generic manner. This change ensures that a single deserialization can be done a descriptor that may be processed in separate contexts. It is unclear whether or not the specification should be pedantic about this, as well. As it is written, these fields are only valid when being referred through a manifest, but this distinction is only required during validation. It may be possible to lift the platform definition to the descriptor type, but this isn't strictly necessary. Signed-off-by: Stephen J Day <stephen.day@docker.com>
832f761 to
572e6ae
Compare
|
@opencontainers/image-spec-maintainers Rebased after the pointer change in to |
|
LGTM |
|
lgtm |
Unifying the descriptor type makes it much easier to handler descriptors
in generic manner. This change ensures that a single deserialization can
be done a descriptor that may be processed in separate contexts.
It is unclear whether or not the specification should be pedantic about
this, as well. As it is written, these fields are only valid when being
referred through a manifest, but this distinction is only required
during validation. It may be possible to lift the platform definition to
the descriptor type, but this isn't strictly necessary.
Signed-off-by: Stephen J Day stephen.day@docker.com
Addresses #588.
Closes #618.