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.

Sign up for GitHub

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

Jump to bottom

Have gengapic protoc plugin invoke samplegen #218

Merged
merged 18 commits into from
Oct 10, 2019
Merged

Have gengapic protoc plugin invoke samplegen #218

merged 18 commits into from
Oct 10, 2019

Conversation

yihanzhen
Copy link
Contributor

@yihanzhen yihanzhen commented Oct 2, 2019 •
edited
Loading

  • Move most of cmd/gen-go-sample to intenal/gensample, but leave main.go still in cmd/gen-go-sample. This makes sure that samplegen can continue to run as a standalone program, and gapic-gen and invoke samplegen

  • Add --sample, --gapic-config and --sample-only flag to docker entry point, and pass them along to gensample and gengapic.

    • When --sample-only is present, toggle off gengapic and gapic_validator_out, so that we can generate samples for APIs that are not yet properly annotated.
    • gensample will consume gapic config and sample config.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2019
@yihanzhen yihanzhen changed the title [WIP] Have gengapic protoc plugin invoke samplegen Have gengapic protoc plugin invoke samplegen Oct 2, 2019
noahdietz
noahdietz reviewed Oct 2, 2019
View reviewed changes
cmd/protoc-gen-go_gapic/main.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
@yihanzhen yihanzhen changed the title Have gengapic protoc plugin invoke samplegen [WIP] Have gengapic protoc plugin invoke samplegen Oct 3, 2019
vchudnov-g
vchudnov-g reviewed Oct 3, 2019
View reviewed changes
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

Initial comments. I know you're in the middle of merging, so I'll TAL after.

cmd/gen-go-sample/main.go Outdated Show resolved Hide resolved
internal/gengapic/client_init_test.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
@yihanzhen
Copy link
Contributor Author

Merge conflicts solved! PTAL

I noticed that github is not showing diffs in gensample.go because it is duplicated from cmd/main.go. I can make a duplicate gensample.go from main.go without any change and submit it as a separate PR, so that the diffs will show up here.

Let me know what you folks think.

@yihanzhen yihanzhen changed the title [WIP] Have gengapic protoc plugin invoke samplegen Have gengapic protoc plugin invoke samplegen Oct 3, 2019
noahdietz
noahdietz reviewed Oct 3, 2019
View reviewed changes
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
vchudnov-g
vchudnov-g reviewed Oct 4, 2019
View reviewed changes
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/plugin.go Outdated Show resolved Hide resolved
internal/gensample/plugin.go Show resolved Hide resolved
@yihanzhen
Copy link
Contributor Author

PTAL

noahdietz
noahdietz reviewed Oct 6, 2019
View reviewed changes
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
vchudnov-g
vchudnov-g reviewed Oct 7, 2019
View reviewed changes
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

The most important comment is about the chan error reading.

internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/gensample.go Outdated Show resolved Hide resolved
internal/gensample/plugin.go Outdated Show resolved Hide resolved
@yihanzhen
Copy link
Contributor Author

PTAL

noahdietz
noahdietz approved these changes Oct 9, 2019
View reviewed changes
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

LGTM. Wait for @vchudnov-g 's review

vchudnov-g
vchudnov-g approved these changes Oct 10, 2019
View reviewed changes
internal/gensample/gensample.go Outdated Show resolved Hide resolved
@yihanzhen yihanzhen merged commit 1929c35 into googleapis:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahdietz noahdietz noahdietz approved these changes

@vchudnov-g vchudnov-g vchudnov-g approved these changes

@software-dov software-dov Awaiting requested review from software-dov

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yihanzhen @vchudnov-g @noahdietz @googlebot