Skip to content
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

Remove --pbout, ensure .pb.go are generated next to .proto #239

Merged
merged 7 commits into from
Nov 5, 2017

Conversation

hasLeland
Copy link
Contributor

Implements / Fixes #236

What does this change?

This change removes the --pbout CLI flag for the truss binary, and changes where truss places the .pb.go files it generates. Instead of placing the .pb.go file(s) into the newly-created NAME-service directory with your other generated code, now truss places the .pb.go file(s) into the same directory as the .proto files which they're generated from.

To better illustrate what those changes mean, let's walk through the old behavior and the new behavior. Let's say you have a directory like this:

$ tree .
.
└── example.proto

First, let's look at the old behavior:

$ truss example.proto
$ tree -L 2
.
├── example.proto
└── example-service
    ├── cmd
    ├── docs
    ├── example.pb.go  <-  Notice that the .pb.go is placed
    ├── handlers           inside of the generated service
    └── svc

As you can see, the example.pb.go file is placed within the example-service directory generated by truss. Now, let's see how this differs from the new behavior, which places the .pb.go files in the same directory as the .proto files which the .pb.go files are generated from.

$ truss example.proto
$ tree -L 2
.                          Notice that the .pb.go file is now placed outside of
├── example.pb.go      <-  generated service directory, alongside the existing
├── example.proto          .proto file which defines the service.
└── example-service
    ├── cmd
    ├── docs
    ├── handlers
    └── svc

In the above, we can see the new behavior which places the .pb.go file next to the .proto file defining the service.

  • Remove the --pbout flag from cmd/main.go
  • Remove the integration tests for --pbout
  • Change .pb.go output to be the same as the .proto output

Copy link
Member

@adamryman adamryman left a comment

Choose a reason for hiding this comment

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

Looks good to me. Though please investigate my comment about using the service definition path. It would be nice to roll that in here.

@@ -191,7 +179,7 @@ func TestAdditionalBindings(t *testing.T) {

func testEndToEnd(defDir string, subcmd string, t *testing.T, trussOptions ...string) {
path := filepath.Join(basePath, defDir)
err := createTrussService(path)
err := createTrussService(path, trussOptions...)
Copy link
Member

Choose a reason for hiding this comment

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

Wait... Was this never testing the --pbout flag? That is kinda bad... Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is kinda bad, though I did fix these tests on master to see if things where broken and they where not.

// so that's where we're going to place them by default. This implies an
// assumption that our .proto files exist within our GOPATH.
//
// Also, if they've passed in multiple protobuf files, we're going to use
Copy link
Member

Choose a reason for hiding this comment

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

In the future, we want to support passing in several protobuf packages. Though only one package will be allowed to have a Service definition.

Is there a way to update this such that protoDir is created from the DefPath of the .proto file that contains the service definition? I know you have that function that tells you what the ServiceName is, so I believe that function could be used/modified to figure out what the path to the .proto file that contains the service definition.

That make sense? It was a bit wordy.

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be added in a future PR.

@adamryman adamryman merged commit d1119d3 into metaverse:master Nov 5, 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