-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fixes. #917. Added include_std_type configuration option. #1136
Conversation
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this!
The change mostly looks good to me, but the plugin-tester-java project now fails because it is also ran as part of the gradle/sbt builds, and this feature is not yet available there. Of course the 'best' way to fix this would be to implement this feature for sbt and gradle as well, but I can imagine that is beyond the scope of this PR. In that case we should perhaps remove comment out the changes to plugin-tester-java, and add tickets to fill in the gradle and sbt support before enabling the test?
a54379e
to
36fb840
Compare
@raboof, I have fixed the gradle buld as well. The sbt looks like a tough one as I have to pass through the options to the sbt-protoc plugin. |
OK TO TEST |
Cool!
Actually you were already almost there - I took the liberty of amending your PR with the final bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments - starting to shape up nicely!
gradle-plugin/src/main/groovy/akka/grpc/gradle/AkkaGrpcPluginExtension.groovy
Outdated
Show resolved
Hide resolved
plugin-tester-java/src/main/java/example/myapp/helloworld/GreeterServer.java
Outdated
Show resolved
Hide resolved
8743e94
to
76ce3e9
Compare
sbt-plugin/src/sbt-test/gen-java/01-gen-basic-java/src/main/protobuf/helloworld.proto
Show resolved
Hide resolved
76ce3e9
to
a85ac4a
Compare
+ Maven + Gradle + Sbt
a85ac4a
to
2fcbed5
Compare
00e84d4
to
ff48e93
Compare
cleanup + version alignment
ff48e93
to
777f90d
Compare
e44cbdc
to
9c6ee53
Compare
9c6ee53
to
ab87b9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last minor comment, then LGTM!
Co-authored-by: Arnout Engelen <github@bzzt.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! (will merge when CI is green)
References #917