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

Merge service and type definitions in protos #827

Merged
merged 12 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@gdbelvin
Collaborator

gdbelvin commented Oct 2, 2017

For some workspace specific reasons, we had split the service and type definitions in our protos.
That reason no longer exists, and the split is causing unneeded complexity between keeping core and impl separate.

@gdbelvin gdbelvin requested review from phad and cesarghali Oct 2, 2017

@phad

phad approved these changes Oct 3, 2017

The package alias nit is something that you might push back on as it would make the diff for this PR much less clean. It could be done some other time, or not at all.

@@ -51,6 +51,7 @@ func mustMetadataAsAny(t *testing.T, meta *keytransparency_v1_types.MapperMetada
// Test vectors were obtained by observing the integration tests, in particular by adding logging
// output around the calls to GetEntry and VerifyGetEntryResponse in grpc_client.go, and the input
// to merkle.VerifyMapInclusionProof in VerifyGetEntryResponse.
// fmt.Printf("%# v", pretty.Formatter(e))

This comment has been minimized.

@phad

phad Oct 3, 2017

Contributor

remove this commented out line?

@phad

phad Oct 3, 2017

Contributor

remove this commented out line?

This comment has been minimized.

@gdbelvin

gdbelvin Oct 3, 2017

Collaborator

I thrashed around trying to remember this line, so I thought I'd include it for posterity.

@gdbelvin

gdbelvin Oct 3, 2017

Collaborator

I thrashed around trying to remember this line, so I thought I'd include it for posterity.

This comment has been minimized.

@phad

phad Oct 3, 2017

Contributor

ok, fair enough.

@phad

phad Oct 3, 2017

Contributor

ok, fair enough.

Show outdated Hide outdated core/internal/protohelpers.go
Show outdated Hide outdated core/keyserver/keyserver.go
Show outdated Hide outdated core/monitor/monitor.go
Show outdated Hide outdated core/mutation/mutation.go
Show outdated Hide outdated core/mutator/entry/entry.go
Show outdated Hide outdated core/proto/monitor_v1/gen.go
Show outdated Hide outdated impl/sql/commitments/sql.go

@google google deleted a comment from codecov-io Oct 3, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 3, 2017

Codecov Report

Merging #827 into master will not change coverage.
The diff coverage is 40.32%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #827   +/-   ##
=======================================
  Coverage   45.56%   45.56%           
=======================================
  Files          34       34           
  Lines        2739     2739           
=======================================
  Hits         1248     1248           
  Misses       1302     1302           
  Partials      189      189
Impacted Files Coverage Δ
impl/monitor/server.go 31.03% <ø> (ø) ⬆️
impl/mutation/mutation.go 33.33% <ø> (ø) ⬆️
integration/testutil.go 66.16% <ø> (ø) ⬆️
core/monitor/sign.go 0% <0%> (ø) ⬆️
core/monitor/monitor.go 0% <0%> (ø) ⬆️
core/client/kt/requests.go 0% <0%> (ø) ⬆️
core/keyserver/keyserver.go 0% <0%> (ø) ⬆️
core/monitor/verify.go 0% <0%> (ø) ⬆️
core/sequencer/sequencer.go 8.9% <0%> (ø) ⬆️
core/mutation/mutation.go 60.12% <100%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faaa8ba...db6e6f8. Read the comment docs.

codecov-io commented Oct 3, 2017

Codecov Report

Merging #827 into master will not change coverage.
The diff coverage is 40.32%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #827   +/-   ##
=======================================
  Coverage   45.56%   45.56%           
=======================================
  Files          34       34           
  Lines        2739     2739           
=======================================
  Hits         1248     1248           
  Misses       1302     1302           
  Partials      189      189
Impacted Files Coverage Δ
impl/monitor/server.go 31.03% <ø> (ø) ⬆️
impl/mutation/mutation.go 33.33% <ø> (ø) ⬆️
integration/testutil.go 66.16% <ø> (ø) ⬆️
core/monitor/sign.go 0% <0%> (ø) ⬆️
core/monitor/monitor.go 0% <0%> (ø) ⬆️
core/client/kt/requests.go 0% <0%> (ø) ⬆️
core/keyserver/keyserver.go 0% <0%> (ø) ⬆️
core/monitor/verify.go 0% <0%> (ø) ⬆️
core/sequencer/sequencer.go 8.9% <0%> (ø) ⬆️
core/mutation/mutation.go 60.12% <100%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faaa8ba...db6e6f8. Read the comment docs.

@gdbelvin gdbelvin merged commit 1b31721 into google:master Oct 3, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gdbelvin gdbelvin deleted the gdbelvin:refactor/proto branch Oct 3, 2017

gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Oct 6, 2017

gdbelvin added a commit that referenced this pull request Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment