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

Apply the protobuf gradle plugin properly #1384

Merged
merged 3 commits into from
Jan 18, 2017
Merged

Conversation

SerialVelocity
Copy link
Contributor

@SerialVelocity SerialVelocity commented Dec 14, 2016

We exclude files that don't exist currently. Should we delete the proto or generate the files?


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Dec 14, 2016

Current coverage is 53.52% (diff: 100%)

Merging #1384 into develop will increase coverage by 0.65%

@@            develop      #1384   diff @@
==========================================
  Files           675        665    -10   
  Lines         41111      40833   -278   
  Methods           0          0          
  Messages          0          0          
  Branches       5906       5882    -24   
==========================================
+ Hits          21735      21854   +119   
+ Misses        17577      17142   -435   
- Partials       1799       1837    +38   

Powered by Codecov. Last update 4264224...dd7dd40

@gsheasby gsheasby modified the milestones: 0.28.0, 0.27.0 Jan 6, 2017
Copy link
Contributor

@jboreiko jboreiko 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, just don't really have a handle on where or how these things are used yet so going to wait on finding out more.

@@ -1,15 +1,6 @@
apply from: "../gradle/publish-jars.gradle"
apply from: "../gradle/shared.gradle"
Copy link
Contributor

Choose a reason for hiding this comment

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

this package (and the leader-election-api-protobuf) are a bit odd in general considering they only exist to contain generated code. I'm wondering if we should just throw it all into there source. I've reached out to @gbonik hoping to find out more about who actually consumes these.

@jboreiko
Copy link
Contributor

jboreiko commented Jan 12, 2017

Cool, just caught up with @rjullman, after poking around. It looks like several of these should only exist internally (where the generated code actually lives). I'll amend the PR to remove them and the excludes for them as well. I will also be filing a ticket for OSS the protobuf based timestamp and lock services (which these use). These protos already exist internally, this simply removes some duplication.

Copy link
Contributor

@jboreiko jboreiko left a comment

Choose a reason for hiding this comment

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

Could clean up the gradle a bit (version strings maybe?) but seems fine and isn't likely to be touched anytime soon.

@jboreiko
Copy link
Contributor

#1449

@gsheasby gsheasby modified the milestones: 0.29.0, 0.28.0 Jan 13, 2017
@jboreiko jboreiko merged commit 40fb75f into develop Jan 18, 2017
@jboreiko jboreiko deleted the fix/protobuf-plugin branch January 18, 2017 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants