Skip to content
This repository was archived by the owner on Oct 11, 2024. It is now read-only.

Replace tinkio.ProtoFile with keyset.BinaryReader #1492

Merged
merged 3 commits into from
Mar 17, 2020
Merged

Conversation

gdbelvin
Copy link
Contributor

@gdbelvin gdbelvin commented Mar 12, 2020

tinkio.ProtoFile can be replaced with the BinaryReader/BinaryWriter

Simplify the crypto library by removing duplicate features.

tinkio.ProtoFile can be replaced with the BinaryReader/BinaryWriter
@gdbelvin gdbelvin requested review from thaidn and a team as code owners March 12, 2020 18:46
@gdbelvin gdbelvin requested a review from Martin2112 March 12, 2020 18:46
@gdbelvin gdbelvin requested review from NatalieDoduc and removed request for Martin2112 March 12, 2020 18:46
@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #1492 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1492      +/-   ##
==========================================
+ Coverage   68.25%   68.27%   +0.01%     
==========================================
  Files          54       53       -1     
  Lines        4026     4000      -26     
==========================================
- Hits         2748     2731      -17     
+ Misses        879      867      -12     
- Partials      399      402       +3
Impacted Files Coverage Δ
core/sequencer/election/tracker.go 70.11% <0%> (-10.35%) ⬇️
core/keyserver/revisions.go 62.58% <0%> (-2.05%) ⬇️
impl/sql/directory/storage.go 67.66% <0%> (-1.51%) ⬇️
core/sequencer/server.go 72.96% <0%> (-0.98%) ⬇️

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 1cc4602...f40a167. Read the comment docs.

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

Is the Build Error expected? Is there another separate PR in flight that will green-ify the build?

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

Can you add in your PR description what motivates the change?

Also - i'm not familiar with the general codebase, but with this being used in many binaries, is it worth to have a keyset(File) handling library?

Finally - a small request for tests?

@NatalieDoduc NatalieDoduc removed their assignment Mar 17, 2020
@gdbelvin
Copy link
Contributor Author

There appears to be a flakey kubernetes test due to an unreliable curl command...

@gdbelvin
Copy link
Contributor Author

The integration tests build and exercise the command line utility that this PR modifies.

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

LGTM

For collective future knowledge, what did you do to re-trigger the flaky curl command?

@gdbelvin gdbelvin merged commit 139f966 into google:master Mar 17, 2020
@gdbelvin
Copy link
Contributor Author

To re-run flakey tests, login to travis.com

@gdbelvin gdbelvin deleted the bio branch March 17, 2020 17:16
gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Mar 25, 2020
* master: (109 commits)
  Upgrade Prometheus (google#1500)
  Run `go build ./...` with github action (google#1446)
  Replace tinkio.ProtoFile with keyset.BinaryReader (google#1492)
  Bump github.com/golang/mock from 1.4.1 to 1.4.2
  Bump github.com/prometheus/client_golang from 1.5.0 to 1.5.1
  Bump github.com/golang/protobuf from 1.3.3 to 1.3.5
  Bump github.com/google/tink from 1.3.0-rc4 to 1.3.0
  New Design Doc (google#1469)
  Acknowledgements (google#1490)
  Use cases (google#1489)
  Bump github.com/prometheus/client_golang from 1.4.1 to 1.5.0 (google#1486)
  README.md # Related (google#1485)
  Explain authorized keys (google#1484)
  Use new prometheus sidecar deployment (google#1483)
  don't overwrite the ReplicaSet service label (google#1482)
  Update encrypted creds (google#1481)
  Pickup DB_HOST env variable (google#1480)
  Fix credential encryption (google#1479)
  Use Cloud MySQL in GKE (google#1473)
  Update client_secrets.json.enc (google#1478)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants