proto: initial protobuf for config #179

vbatts · 2015-09-11T17:05:42Z

To have a less source based, and more consumable example of structures,
this is an initial pass at protobuf structures for the structures in config.go

There is some exercise needed in platform specific structures.

For the sake of example, the Makefile defaults to outputing golang
source, but has a 'c' target (make c) for C source as well.

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

vbatts · 2015-09-11T17:05:57Z

cc @philips @LK4D4 @vishh @crosbymichael

mrunalp · 2015-09-11T17:12:59Z

👍 We should probably create a branch to flesh this out and see if it meets all our requirements.

vbatts · 2015-09-11T17:17:29Z

fair. I'm still working through it, but figured to get the conversation
started.

On Fri, Sep 11, 2015 at 1:13 PM, Mrunal Patel [email protected]
wrote:

[image: 👍] We should probably create a branch to flesh this out and
see if it meets all our requirements.

—
Reply to this email directly or view it on GitHub
#179 (comment).

vishh · 2015-09-11T17:36:34Z

proto/config.proto

One thing to consider with required fields is that it might be almost impossible to make it optional.

are you saying to start with preferring optional?

crosbymichael · 2015-09-11T18:27:24Z

Ya, we could do a branch on the repo to figure this out if that makes it easier.

vbatts · 2015-09-11T18:44:35Z

@vishh updated the field rules

vbatts · 2015-09-11T18:45:24Z

So, I've stubbed out the [platform dependent] User, in the best pattern I can derive from the guidelines and references. Thoughts?

wking · 2015-09-11T19:43:46Z

proto/config.proto

Is this going to give us per-platform namespacing inside User? E.g.

"user": { "linux": { "uid": 0, "gid": 0, } }

That's the pattern we use in other sections of the existing config (e.g. for linux.capabilities), but it seems excessive here. However, the benefits of a protobuf-based schema may warrant a few warts in the associated JSON structure.

not namespacing, just platform dependent user attributes.

On Fri, Sep 11, 2015 at 12:58:22PM -0700, Vincent Batts wrote:

// LinuxUserFields are to be used when Type is LINUX

optional linuxUserFields LinuxUserFields = 2;

not namespacing, just platform dependent user attributes.

Generated Go (from b5433ce) has:

// User specifies user information for the process.
type User struct {
// Type so that receivers of this message can switch for the fields expected
Type *PlatformType protobuf:"varint,1,opt,enum=oci.PlatformType,def=1" json:"Type,omitempty"
// LinuxUserFields are to be used when Type is LINUX
LinuxUserFields *LinuxUserFields protobuf:"bytes,2,opt" json:"LinuxUserFields,omitempty"
XXX_unrecognized []byte json:"-"
}

Which looks like it's going to lead to a an explicit “LinuxUserFields”
level to me. And again, I'm not against a bit of wordiness in support
of spec that can autogenerate bindings in a number of languages, but
that explicit platform level is not how we handle user entries now.

Any primitive might be useful here.

@vishh ah that sounds like what I'm looking for, but that's in proto3. I'm dealing with proto2 :-\

I'm playing with permutations of extensions right now

@wking general adoption

On Mon, Sep 14, 2015 at 10:06:25AM -0700, Vincent Batts wrote:

@wking general adoption

Does protobuf-version adoption really matter? If we're just using it
for code-generation, it seems like you'd only need a few folks running
the protobuf-to-$X translators. Everyone else can just consume that
generated code, without interacting with protobuf at all.

I don't intend on commiting the generated code. So yes, it would depend on the version of protobuf compiler available to the user

On Mon, Sep 14, 2015 at 12:04:48PM -0700, Vincent Batts wrote:

I don't intend on commiting the generated code.

No need to commit it, just ship it with release tarballs.

vbatts · 2015-09-11T20:14:13Z

Alrighty, y'all. A couple of concessions, but is pretty much lined up for an initial review.

wking · 2015-09-11T20:17:07Z

proto/Makefile

‘defualt’ → ‘default’ ;)

vishh · 2015-09-11T20:39:46Z

proto/config.proto

If any of these fields are required to be set, can we explicitly state that in the comment?

Yeah. That relationship is a bit cloudied by the required, optional, repeated field rules.

To have a less source based, and more consumable example of structures, this is an initial pass at protobuf structures for the structures in config.go There is some exercise needed in platform specific structures. For the sake of example, the Makefile defaults to outputing golang source, but has a 'c' target (`make c`) for C source as well. Signed-off-by: Vincent Batts <[email protected]>

This User structure does not map to the cleanliness of the current go structure, but will allow the definition to be all in one place, regardless of the host that is doing the compilation. Switching field rules to `optional` for now. Per vish and the docs, https://developers.google.com/protocol-buffers/docs/proto?csw=1#specifying-field-rules "! Required Is Forever" Also add a `make py` and `make all` target. Signed-off-by: Vincent Batts <[email protected]>

There are a couple of concessions, but is pretty much lined up. Signed-off-by: Vincent Batts <[email protected]>

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

Let the user decide the value. opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

per opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

vbatts · 2015-09-14T17:09:06Z

updated for comments.

Also, I'll have to close this PR and open another to put these changes against a branch besides master. Not sure where/what we want that branch to be. dev? protobuf?

mrunalp · 2015-09-14T17:10:07Z

@vbatts Maybe just call it protobuf.

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

vbatts · 2015-09-14T17:30:41Z

@vishh so, i just pushed an iteration of User as an extension. The godoc is here. LMKWYT

vbatts · 2015-09-14T17:41:19Z

moving this PR over to #185

Let the user decide the value. opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

per opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

vishh reviewed Sep 11, 2015
View reviewed changes

vbatts mentioned this pull request Sep 11, 2015

[Tracker] Platform/Compiler agnostic schema #166

Closed

wking reviewed Sep 11, 2015
View reviewed changes

vbatts force-pushed the protobuf branch from 738306f to b5433ce Compare September 11, 2015 20:13

wking reviewed Sep 11, 2015
View reviewed changes

proto/Makefile Outdated

Copy link

Contributor

wking Sep 11, 2015

Choose a reason for hiding this comment

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

‘defualt’ → ‘default’ ;)

vbatts force-pushed the protobuf branch from b5433ce to 76b8006 Compare September 11, 2015 20:22

vishh reviewed Sep 11, 2015
View reviewed changes

wking mentioned this pull request Sep 12, 2015

.tools: Add per-commit Go checks (vet, fmt, and lint) #182

Closed

vbatts added 9 commits September 14, 2015 10:30

proto: runtime and linux runtime done

beabf64

There are a couple of concessions, but is pretty much lined up. Signed-off-by: Vincent Batts <[email protected]>

proto/config: Explicit linux user message

113bfe7

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

proto/config: style guide and indentation

32e3ec9

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

proto/config: indentation

c68fac1

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

proto: changes per style guide

eb48ef2

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

proto: remove default values

fa1ae38

Let the user decide the value. opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

proto: default expanded enum to unknown

9adb993

per opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

vbatts force-pushed the protobuf branch from 76b8006 to 9adb993 Compare September 14, 2015 17:07

proto/config: User as extensions

e8d8934

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

vbatts closed this Sep 14, 2015

vbatts added a commit to vbatts/oci-runtime-spec that referenced this pull request Sep 15, 2015

proto: remove default values

0373e8f

Let the user decide the value. opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

vbatts added a commit to vbatts/oci-runtime-spec that referenced this pull request Sep 15, 2015

proto: default expanded enum to unknown

eb1d649

per opencontainers#179 (comment) Signed-off-by: Vincent Batts <[email protected]>

proto: initial protobuf for config #179

proto: initial protobuf for config #179

Uh oh!

Conversation

vbatts commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

mrunalp commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts Sep 14, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 14, 2015

Uh oh!

mrunalp commented Sep 14, 2015

Uh oh!

vbatts commented Sep 14, 2015

Uh oh!

vbatts commented Sep 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants