Skip to content

spec: Replace incorrect use of maybe with MAY be #108

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

Conversation

gpaul
Copy link

@gpaul gpaul commented Sep 20, 2017

This PR fixes three incorrect uses of the word "maybe" where "may be" was intended.

@jdef
Copy link
Member

jdef commented Sep 20, 2017

Thanks for the PR. Can you run make to rebuild the generated .proto and related stubs?

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

Done

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

May I ask how you're editing the spec.md file without your editor correcting the line endings? What OS/editor are you using? I've tried both Vim and Atom on macOS and they both insist on "correcting" the whitespace. It's the reason I filed #111.

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

Oh - interesting - I wanted to make a tiny change so I just used nano (from linux).

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @jdef,

Remember when you, @saad-ali, and @jieyu advised holding off on making the generation process dependent upon certain versions of Go libraries via Glide or dep? Well, it looks like it's finally biting us in the process's behind. @gpaul's changes are failing on Travis-CI:

make: Entering directory `/home/travis/gopath/src/github.com/container-storage-interface/spec/lib/go'
./protoc -I "../.." --go_out=plugins=grpc:"csi/.build" "../../csi.proto"
diff "csi/csi.pb.go" "csi/.build/csi.pb.go"
1c1
< // Code generated by protoc-gen-go.
---
> // Code generated by protoc-gen-go. DO NOT EDIT.
3d2
< // DO NOT EDIT!
make: *** [csi/csi.pb.go] Error 1
make: Leaving directory `/home/travis/gopath/src/github.com/container-storage-interface/spec/lib/go'

I believe it's because @gpaul needs to update his version of github.com/golang/protobuf/protoc-gen-go since the Makefile does not manage the version used in the CI process -- it just go gets it with go get -d. This command of course does not update that package if it already exists. On Travis-CI, however, it's always the latest version since it's a clean GOPATH.

I did specify a version for protoc -- 3.3.0 currently -- but for Go dependencies y'all didn't want me to manage their versions via a manifest or a committed vendor tree at the time.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

nano huh? Isn't that just a fancy pico? I always thought vim was even cleaner in terms of sparseness. How about your OS? If you're using nano I imagine it's some Linux distro, right?

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

Spot on, I'm running fedora 26.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

Can you please run go get -u github.com/golang/protobuf/protoc-gen-go locally and then re-run make to regenerate the files? I hope this is the issue as it's what I outlined above as the probable cause for the continued build failures.

Also, do you mind squashing the commits? Thanks!

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

Can you please run go get -u github.com/golang/protobuf/protoc-gen-go locally ... ?

I'm tempted to say no. This makes no sense.

Generated files need to be generated in a standard docker with the Dockerfile committed to the repo. I actually quite like the version of protoc-gen-go I have right now.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

I agree completely. I originally wanted to version the build tools either via manifest or committing vendor with the lib/go package. However, I was advised to keep things simple and just always use the latest version of the Go packages until something broke.

My response was at the time, and is still today, "simple isn't better if it breaks." Well, it broke.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

Also, Docker isn't strictly necessary (although it would make it entirely deterministic) as you can easily track the required tools via a dep manifest or by committing vendor and then building the tools from vendor -- which of course will use the required version.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

There are two other reasons I elected not to use Docker:

  1. Using Docker doesn't by itself guarantee a specific version of the build tools, especially if they are built from Go packages. You still need to manage their source versions with a manifest or by committing vendor.
  2. Running Docker on Travis-CI requires a sudo: required build which means your builds cannot use their container infrastructure and require full VMs. This increases build queue times exponentially in my experience, and I'd like to say I've plenty of it at this point :)

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

Yeah. I'm happy to wait with this PR until we have something better. I'm happy to contribute to that effort. I advocate for embracing dep and committing the manifest file. Normally I would commit the vendor directory but I guess the transitive dependency list is pretty huge and go-specific.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

I'm happy to contribute to that effort. I advocate for embracing dep and committing the manifest file.

I already have a branch that uses dep waiting for the moment the current model broke.

Normally I would commit the vendor directory but I guess the transitive dependency list is pretty huge and go-specific.

It's not small. See GoCSI's vendor directory.

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

For this I'm totally happy with just using dep without docker. Docker would give you a specific go version and specific protobuf-compiler version, but I guess "we can fix that when that breaks".

You having a branch ready is pretty classic :D My wife and I had a good chuckle.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

Well, seeing as how gRPC's Golang implementation still refuses to adopt the Go stdlib context.Context type because it breaks backwards compatibility with people using golang.org/x/net/context.Context I am less concerned about the version of Go. I know the gRPC Golang team are currently waiting on Go 1.9 (which has shipped) and its new Type alias mechanic to update to the new stdlib context.Context type while still maintaining backwards compatibility.

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

I go to submit my branch as a PR after updating it to the latest version of dep and this happens :(

@gpaul
Copy link
Author

gpaul commented Sep 20, 2017

This comic comes to mind: https://xkcd.com/1172/

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

Absolutely!

--
-a

p.s. Except when it's my workflow that's broken -- then it definitely should be fixed ;)

@akutz
Copy link
Contributor

akutz commented Sep 20, 2017

Hi @gpaul,

FWIW, the current implementation already guarantees a version of the protoc compiler. It's currently set to 3.3.0 in the Makefile.

@akutz akutz mentioned this pull request Sep 20, 2017
@jdef
Copy link
Member

jdef commented Nov 14, 2017

closing this out since #115 rewrote the error handling part of the spec.

@jdef jdef closed this Nov 14, 2017
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.

3 participants