KEMBAR78
Remove log_root_signature from SignedLogRoot. by pphaneuf · Pull Request #2452 · google/trillian · GitHub
Skip to content

Remove log_root_signature from SignedLogRoot. #2452

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
Merged
merged 2 commits into from
Apr 8, 2021
Merged

Remove log_root_signature from SignedLogRoot. #2452

merged 2 commits into from
Apr 8, 2021

Conversation

@pphaneuf
Copy link
Contributor

@pphaneuf pphaneuf commented Apr 7, 2021

Checklist

@google-cla google-cla bot added the cla: yes label Apr 7, 2021
@codecov
Copy link

codecov bot commented Apr 7, 2021 •
edited
Loading

Codecov Report

Merging #2452 (9ae5cba) into master (1cfd1ba) will decrease coverage by 0.09%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
- Coverage   66.15%   66.06%   -0.10%     
==========================================
  Files         104      104              
  Lines        7166     7146      -20     
==========================================
- Hits         4741     4721      -20     
+ Misses       1916     1915       -1     
- Partials      509      510       +1
Impacted Files Coverage Δ
crypto/signer.go 91.66% <ø> (+4.71%) ⬆️
log/sequencer.go 73.91% <66.66%> (-1.82%) ⬇️
storage/tools/dump_tree/dumplib.go 40.94% <66.66%> (-0.26%) ⬇️
server/log_rpc_server.go 84.61% <100.00%> (-0.15%) ⬇️
storage/cloudspanner/log_storage.go 55.70% <100.00%> (-0.27%) ⬇️
storage/mysql/log_storage.go 65.61% <100.00%> (-0.25%) ⬇️
log/operation_manager.go 88.35% <0.00%> (+1.05%) ⬆️

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 1cfd1ba...9ae5cba. Read the comment docs.

@pphaneuf pphaneuf changed the title No signed root Remove log_root_signature from SignedLogRoot. Apr 7, 2021
@pphaneuf pphaneuf requested review from Martin2112 and pav-kv April 8, 2021 12:55
@pphaneuf
Copy link
Contributor Author

pphaneuf commented Apr 8, 2021

There's two commits in this PR, it might help a bit to look at them separately?

@pphaneuf pphaneuf requested a review from a team April 8, 2021 14:24
pav-kv
pav-kv approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

LGTM. Reviewing it as a whole was simpler, as the second commit deleted stuff added in the first one.

@pphaneuf
Copy link
Contributor Author

pphaneuf commented Apr 8, 2021

Yeah, the overall diff might be smaller, but the separate commits make it maybe easier to understand (inline the uses of SignLogRoot in step 1, then remove the signing in step 2). Good to go now, thanks!

@pphaneuf pphaneuf marked this pull request as ready for review April 8, 2021 14:40
@pphaneuf pphaneuf requested a review from AlCutter as a code owner April 8, 2021 14:40
@pphaneuf pphaneuf merged commit 884ef51 into google:master Apr 8, 2021
@pphaneuf pphaneuf deleted the no_signed_root branch April 8, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Martin2112 Martin2112 Awaiting requested review from Martin2112

@AlCutter AlCutter Awaiting requested review from AlCutter AlCutter is a code owner

1 more reviewer

@pav-kv pav-kv pav-kv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pphaneuf @pav-kv