Skip to content

Conversation

@vbatts
Copy link
Member

@vbatts vbatts commented Oct 6, 2016

there are annotations in the manifest, but having the same path
available for configuration specifc annotations seems logical as well.

Relates to #371

Signed-off-by: Vincent Batts [email protected]

@runcom
Copy link
Member

runcom commented Oct 6, 2016

+1

@jonboulle
Copy link
Contributor

LGTM but I wonder more and more about the purpose/sanity of this config
object - does it still reflect what's in a modern docker image?does it make
any sense to define it here without a process to convert between it and the
runtime spec? (I think this conversation has already started on another
issue but imo the topic is quite unresolved)

Antonio Murdaca [email protected] schrieb am Do., 6. Okt. 2016,
10:25:

+1


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#372 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACewNz2me-RI1ybzXXi-czNFslvY4uFMks5qxLBvgaJpZM4KPq8d
.

@vbatts
Copy link
Member Author

vbatts commented Oct 6, 2016

this particular piece is overlapped with #371, as the .config.Labels field is existing in the Docker config, even though is unspecified.
Does this annotations accomplish something separate from Labels? no, not really.
Is this annotations more consistent with the runtime and image manifests? yes.

I'm two ways about it.

there are annotations in the manifest, but having the same path
available for configuration specifc annotations seems logical as well.

Signed-off-by: Vincent Batts <[email protected]>
@vbatts vbatts force-pushed the config_annotations branch from 5ace02c to 5e528fb Compare October 6, 2016 13:13
@wking
Copy link
Contributor

wking commented Oct 6, 2016

On Thu, Oct 06, 2016 at 01:40:03AM -0700, Jonathan Boulle wrote:

LGTM but I wonder more and more about the purpose/sanity of this
config object - does it still reflect what's in a modern docker
image?does it make any sense to define it here without a process to
convert between it and the runtime spec?

There is discussion on the conversion process and sanity thereof in
#87, which was moved to opencontainers/image-tools#25 although I think
whether we want an image-spec-specific config and how it gets
translated are still image-spec issues 1. And my personal
preference is to just ship a runtime-spec config.json and sanitize it
on unpacking (opencontainers/runtime-tools#219).

But as long as we have an image-spec-specific config based on Docker's
implementation, I think it makes sense to have something that supports
lossless translation (so either this PR or #371). I don't think
translating between field names will be a big deal, but there has been
push-back against translating in the past 2.

@philips
Copy link
Contributor

philips commented Oct 7, 2016

Can we hold on this until post v1.0.0? Is there urgency here?

@vbatts
Copy link
Member Author

vbatts commented Oct 19, 2016

@philips well, this is discovered as I'm working with folks to have OCI image support, and where to stash their arbitrary data. Can they just make the field themselves and do this? sure.
But this would keep the wording consistent across all the OCI annotations usage.

@philips
Copy link
Contributor

philips commented Oct 24, 2016

@vbatts why does it need to be in the config and not the manifest?

@vbatts
Copy link
Member Author

vbatts commented Oct 25, 2016

only urgency is that it makes more sense in the config, than the manifest.
but also consistency with the runtime-spec config.

On Mon, Oct 24, 2016 at 3:07 PM, Brandon Philips [email protected]
wrote:

@vbatts https://github.com/vbatts why does it need to be in the config
and not the manifest?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#372 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6ZJ6xzFpfWd58CCqZnqE-EjoFSXRks5q3QILgaJpZM4KPq8d
.

@vbatts vbatts modified the milestones: v1.0.0-rc3, post-v1.0.0 Oct 28, 2016
@vbatts
Copy link
Member Author

vbatts commented Oct 28, 2016

even though I want to see this consistency, I'm tagging this for post-v1.0.0.
LABELS (#371) needs to be in for v1 for compatibility though.

@vbatts
Copy link
Member Author

vbatts commented Feb 7, 2017

closing this for now. The config has labels, which is effectively equivalent and is consistent with current docker implementation.

@vbatts vbatts closed this Feb 7, 2017
@vbatts vbatts deleted the config_annotations branch February 7, 2017 21:28
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.

5 participants