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

Support generating .pb.go files from protobufs without service definitions #246

Merged
merged 6 commits into from
Dec 15, 2017

Conversation

eriktate
Copy link
Contributor

Effectively just making sure protoc is still run against .proto files that define shared messages and no RPC service.

This PR addresses #242

truss/config.go Outdated
@@ -18,4 +18,7 @@ type Config struct {
DefPaths []string
// The files of a previously generated service, may be nil
PrevGen map[string]io.Reader

// The flag that specifies if a service should be built.
NoService bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it would be preferable for parseInput to return an additional value (NoService) rather than extending the Config struct I can rework that.

@adamryman
Copy link
Member

Hey @eriktate, I'm going to review this in the next few hours. I'm sorry for causing a conflict.

I'll PR a fix on top of your PR, to resolve the conflict. We just wanted the 1.9 fix in very strongly.

Though your PR came up first, so it is only polite for me to resolve this conflict I created.

Thanks for the PR! I look forward to taking a closer look very shortly.

Updated code path based on changes from master
@adamryman
Copy link
Member

Hey @eriktate, here is that PR I mentioned: eriktate#3

Changes look good! Sorry that some of the same logic was reworked right after you put this PR up.

Hope to talk with you soon.

@eriktate
Copy link
Contributor Author

@adamryman No worries! Just merged your PR resolving the conflicts, so this should be good to go!

@adamryman
Copy link
Member

Just because I make some extra changes in the PR on your PR, I would like one other team member to take a look.

@zaquestion, @lelandbatey, if either of you could give a ✅ then I think we will be ready to merge.

Copy link
Member

@lelandbatey lelandbatey left a comment

Choose a reason for hiding this comment

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

Yay for the fix toward #242!

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