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

Simplify command line client flags #738

Merged
merged 6 commits into from Aug 12, 2017

Conversation

Projects
None yet
3 participants
@gdbelvin
Collaborator

gdbelvin commented Aug 9, 2017

With the transition onto Trillian, several of the previous flags are now obsolete.
This PR cleans up and clarifies command line flags.  

This PR also fixes a few things in the client and cleans up docker-compose.

References:
#712
Closes #659

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 9, 2017

Codecov Report

Merging #738 into master will increase coverage by 0.04%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #738      +/-   ##
=========================================
+ Coverage   48.66%   48.7%   +0.04%     
=========================================
  Files          30      30              
  Lines        2503    2505       +2     
=========================================
+ Hits         1218    1220       +2     
  Misses       1088    1088              
  Partials      197     197
Impacted Files Coverage Δ
integration/testutil.go 68.88% <100%> (ø) ⬆️
core/client/kt/verify.go 52.63% <80%> (+1.28%) ⬆️

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 c459b76...f5ee1f9. Read the comment docs.

codecov-io commented Aug 9, 2017

Codecov Report

Merging #738 into master will increase coverage by 0.04%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #738      +/-   ##
=========================================
+ Coverage   48.66%   48.7%   +0.04%     
=========================================
  Files          30      30              
  Lines        2503    2505       +2     
=========================================
+ Hits         1218    1220       +2     
  Misses       1088    1088              
  Partials      197     197
Impacted Files Coverage Δ
integration/testutil.go 68.88% <100%> (ø) ⬆️
core/client/kt/verify.go 52.63% <80%> (+1.28%) ⬆️

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 c459b76...f5ee1f9. Read the comment docs.

@gdbelvin gdbelvin changed the title from WIP: Simplify Flags to Simplify command line client flags Aug 10, 2017

@gdbelvin gdbelvin removed the WIP label Aug 10, 2017

@gdbelvin gdbelvin requested a review from Liamsi Aug 10, 2017

@Liamsi

LGTM besides a few nits:

  • there seems to be copy&paste error in the prepare_client.sh script
  • maybe delete the provided yaml file (?)
Show outdated Hide outdated scripts/prepare_client.sh
KTURL="${KTURLDEFAULT}"
read -p "Trillian Map verification key: " MAPKEY
if [[ -n "${MAPKEY}" ]]; then
printf "map-key: %s\n" "${LOGKEY}" >> .keytransparency.yaml

This comment has been minimized.

@Liamsi

Liamsi Aug 10, 2017

Contributor

Change to ${MAPKEY}?

@Liamsi

Liamsi Aug 10, 2017

Contributor

Change to ${MAPKEY}?

This comment has been minimized.

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

thanks

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

thanks

Show outdated Hide outdated .keytransparency.yaml
kt-url: "35.184.134.53:8080"
client-secret: "client_secret.json"
service-key: ""

This comment has been minimized.

@Liamsi

Liamsi Aug 10, 2017

Contributor

Remove this new line.

@Liamsi

Liamsi Aug 10, 2017

Contributor

Remove this new line.

This comment has been minimized.

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

done

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

done

Show outdated Hide outdated .keytransparency.yaml
kt-url: localhost:8080
client-secret:
service-key:

This comment has been minimized.

@Liamsi

Liamsi Aug 10, 2017

Contributor

This file looks like it does not work as it is. Also, and it gets completely overwritten below (in prepare_client.sh).
Is it still useful? Or should we delete it completely?

@Liamsi

Liamsi Aug 10, 2017

Contributor

This file looks like it does not work as it is. Also, and it gets completely overwritten below (in prepare_client.sh).
Is it still useful? Or should we delete it completely?

This comment has been minimized.

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

This empty file allows the command line client to use the defaults specified in its command line flags, which do work - and it removes another place where we need to manage defaults.

@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

This empty file allows the command line client to use the defaults specified in its command line flags, which do work - and it removes another place where we need to manage defaults.

This comment has been minimized.

@Liamsi

Liamsi Aug 12, 2017

Contributor

Thanks for the explanation. LGTM

@Liamsi

Liamsi Aug 12, 2017

Contributor

Thanks for the explanation. LGTM

@gdbelvin

This comment has been minimized.

Show comment
Hide comment
@gdbelvin

gdbelvin Aug 11, 2017

Collaborator

Updated PTAL

Collaborator

gdbelvin commented Aug 11, 2017

Updated PTAL

@Liamsi

Liamsi approved these changes Aug 12, 2017

@Liamsi Liamsi merged commit bb9607e into google:master Aug 12, 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:polish/flags branch Aug 14, 2017

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