protobuf idioms by bufdev · Pull Request #48 · containerd/containerd

bufdev · 2015-12-18T11:30:50Z

This PR does a few things:

vim gg=G on api.proto
updates api.proto for a few protobuf idioms, namely that repeated fields are named as singular, underscore instead of camel case, and a longer package name but with the go_packageoption applied
makes common messages types.IO, types.ProcessInfo. These may or may not be something you like, and also the naming needs work.
updates the gocode to reflect these changes

bufdev · 2015-12-18T12:11:43Z

If the maintainers are open to it, I'd love to provide some more suggestions on the protobuf API as well :)

LK4D4 · 2015-12-18T16:46:03Z

@peter-edge do you have vim config line for .proto files?

bufdev · 2015-12-18T16:48:57Z

https://github.com/google/protobuf/blob/master/editors/proto.vim

On Friday, December 18, 2015, Alexander Morozov [email protected]
wrote:

@peter-edge https://github.com/peter-edge do you have vim config line
for .proto files?

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48#issuecomment-165832661.

LK4D4 · 2015-12-18T16:50:13Z

@peter-edge no, I mean like ts and stuff :) My default config uses tabs for it. And even my spaces is always 4.

LK4D4 · 2015-12-18T16:51:24Z

Also, I'd prefer each commit to be buildable for later bisects. So makes sense to squash last two commits.

bufdev · 2015-12-18T16:52:18Z

Oh haha. Honestly I don't remember and am away from keyboard right now :)
I'll check my vimrc when I get back (or if you're in a rush, it's in
https://github.com/peter-edge/dotfiles)

On Friday, December 18, 2015, Alexander Morozov [email protected]
wrote:

@peter-edge https://github.com/peter-edge no, I mean like ts and stuff
:) My default config uses tabs for it. And even my spaces is always 4.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48#issuecomment-165834897.

bufdev · 2015-12-18T16:54:48Z

Will also squash when I get back :) also these PRs can be part of a bigger
design proposal, I really made them as suggestions on the direction of the
containerd api (I sometimes think it's beneficial to take the few minutes
and do the code so contributors can get a sense of a proposal). If you like
them, we can merge, or I'm happy to help on a larger PR.

On Friday, December 18, 2015, Peter Edge [email protected] wrote:

Oh haha. Honestly I don't remember and am away from keyboard right now :)
I'll check my vimrc when I get back (or if you're in a rush, it's in
https://github.com/peter-edge/dotfiles)

On Friday, December 18, 2015, Alexander Morozov <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:

@peter-edge https://github.com/peter-edge no, I mean like ts and stuff
:) My default config uses tabs for it. And even my spaces is always 4.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48#issuecomment-165834897.

LK4D4 · 2015-12-18T16:58:47Z

@peter-edge it's always better to post smaller PR. Don't underestimate maintainers laziness :)

bufdev · 2015-12-18T17:03:12Z

Ha. Well I'll take a straightforward abiet lazy maintainer over a volatile
one any day :)

On Friday, December 18, 2015, Alexander Morozov [email protected]
wrote:

@peter-edge https://github.com/peter-edge it's always better to post
smaller PR. Don't underestimate maintainers laziness :)

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48#issuecomment-165838892.

LK4D4 · 2015-12-18T17:08:13Z

Changes LGTM

Signed-off-by: Peter Edge <[email protected]> Update code to reflect api.prot

bufdev · 2015-12-19T22:06:09Z

Last two commits were squashed into first commit. As to the formatting - I believe proto standard is 2 spaces and I think that's what I normally see (and it is what I always use), but I can poke around more to verify that.

stevvooe · 2016-01-05T20:18:21Z

Spacing wars? Really?

Not wars, but we need some style.

This covers some of it: https://developers.google.com/protocol-buffers/docs/style. Spaces are not covered.

Should we follow https://github.com/google/protobuf/blob/master/conformance/conformance.proto as closely as possible? Seems like 2 space indents but they don't have any convention on lining up fields.

I don't think I am going to be able to work outside of gofmt anymore. It is just too cozy.

Yeah, it's superhard without gofmt :/

I just lazily do vim gg=G but ya it messes up sometimes

On Tuesday, January 5, 2016, Alexander Morozov [email protected]
wrote:

In api/grpc/types/api.proto
https://github.com/docker/containerd/pull/48#discussion_r48894868:

}

message CreateContainerRequest {

string id = 1; // ID of container

string bundlePath = 2; // path to OCI bundle

string stdin = 3; // path to the file where stdin will be read (optional)

string stdout = 4; // path to file where stdout will be written (optional)

string stderr = 5; // path to file where stderr will be written (optional)

string console = 6; // path to the console for a container (optional)

string checkpoint = 7; // checkpoint name if you want to create immediate checkpoint (optional)

string id = 1; // ID of container

Yeah, it's superhard without gofmt :/

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48/files#r48894868.

bufdev · 2016-01-17T22:33:03Z

Hey, any updates on this? I'm assuming no longer wanted?

bufdev · 2016-01-21T21:01:52Z

ping @LK4D4 heh

LK4D4 · 2016-01-21T21:03:59Z

@peter-edge I actually posted my LGTM
ping @crosbymichael

bufdev · 2016-01-21T21:05:10Z

Ah my bad. I don't want to leave PRs in your world without them being taken
care of, was doing some clean up :)

On Thu, Jan 21, 2016 at 4:04 PM, Alexander Morozov <[email protected]

wrote:

@peter-edge https://github.com/peter-edge I actually posted my LGTM
ping @crosbymichael https://github.com/crosbymichael

—
Reply to this email directly or view it on GitHub
https://github.com/docker/containerd/pull/48#issuecomment-173707136.

bufdev · 2016-01-21T21:05:19Z

Also @LK4D4 you might like https://github.com/peter-edge/pb

bufdev · 2016-01-21T21:07:46Z

Oh wait, I already linked you to that over in golang/protobuf, lol

crosbymichael · 2016-02-25T18:41:44Z

The way things are moving right now I think we ware going to have to wait for this until we are out of alpha and the apis have stabilized. Thanks for the PR but I'll close this one now so you don't have to worry about rebases and such as we are making a bunch of changes.

Thanks again!

Hardcode libexec paths and var lib paths (fixes SLES)

bufdev mentioned this pull request Dec 18, 2015

Add types.EventType, use in supervisor package #50

Closed

Update api.proto for protobuf idioms, formatting, common messages

846e25f

Signed-off-by: Peter Edge <[email protected]> Update code to reflect api.prot

stevvooe reviewed Jan 5, 2016
View reviewed changes

crosbymichael closed this Feb 25, 2016

jseba pushed a commit to jseba/containerd that referenced this pull request Aug 25, 2023

Merge pull request containerd#48 from seemethere/fix_sles

edb2083

Hardcode libexec paths and var lib paths (fixes SLES)

dmcgowan mentioned this pull request Oct 5, 2023

[release/1.7] Prepare release notes for v1.7.7 #9194

Merged

Conversation

bufdev commented Dec 18, 2015

Uh oh!

bufdev commented Dec 18, 2015

Uh oh!

LK4D4 commented Dec 18, 2015

Uh oh!

bufdev commented Dec 18, 2015

Uh oh!

LK4D4 commented Dec 18, 2015

Uh oh!

LK4D4 commented Dec 18, 2015

Uh oh!

bufdev commented Dec 18, 2015

Uh oh!

bufdev commented Dec 18, 2015

Uh oh!

LK4D4 commented Dec 18, 2015

Uh oh!

bufdev commented Dec 18, 2015

Uh oh!

LK4D4 commented Dec 18, 2015

Uh oh!

bufdev commented Dec 19, 2015

Uh oh!

stevvooe Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

LK4D4 Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

LK4D4 Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

bufdev Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

bufdev commented Jan 17, 2016

Uh oh!

bufdev commented Jan 21, 2016

Uh oh!

LK4D4 commented Jan 21, 2016

Uh oh!

bufdev commented Jan 21, 2016

Uh oh!

bufdev commented Jan 21, 2016

Uh oh!

bufdev commented Jan 21, 2016

Uh oh!

crosbymichael commented Feb 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants