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

Conversation

@liamsi
Copy link
Contributor

@liamsi liamsi commented Aug 17, 2017

Changes:

  • simplify docker files to be "unopinionated"/containing only the bare minimum flags to run them locally
  • remove obsolete libtool dependency (faster builds) 🎉
  • update some defaults

It could make sense to wait for #762 to be merged first (removes --log-key flag).

Tested locally. Travis fails because of some dependency in vendored code in trillian:
https://travis-ci.org/google/keytransparency/builds/265725916#L639-L640

Fixed with google/trillian@a1b0c2d and google/trillian@b144701

closes #712

@liamsi liamsi added the WIP label Aug 17, 2017
@codecov-io
Copy link

codecov-io commented Aug 17, 2017

Codecov Report

Merging #763 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #763   +/-   ##
=======================================
  Coverage   48.42%   48.42%           
=======================================
  Files          28       28           
  Lines        2480     2480           
=======================================
  Hits         1201     1201           
  Misses       1093     1093           
  Partials      186      186

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 bef78fc...26a5a8b. Read the comment docs.

@liamsi liamsi removed the WIP label Aug 17, 2017
@liamsi liamsi changed the title WIP: simplify and update docker files Simplify, cleanup and update docker files Aug 17, 2017
@liamsi liamsi requested a review from gdbelvin August 17, 2017 22:58
RUN apt-get update && apt-get install -y libtool libltdl-dev
RUN go get -tags="mysql" ./cmd/keytransparency-server

ENTRYPOINT /go/bin/keytransparency-server \
Copy link
Contributor

@gdbelvin gdbelvin Aug 18, 2017

Choose a reason for hiding this comment

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

We need to set an entry point. Otherwise, it's not possible to add additional arguments via the docker command line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker-compose and kubernetes work though. I'll add an ENTRYPOINT in case someone wants to use the images with any these. Thanks

- --map-id=$MAP_ID
- --map-url=trillian-map:8090
- --vrf=/kt/vrf-key.pem
- --key=/kt/server.key
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind renaming this flag and the following to tls-key and tls-cert ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@gdbelvin gdbelvin left a comment

Choose a reason for hiding this comment

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

Do the Kube scripts need to be updated as well?

gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Aug 18, 2017
Cleans up a few off by on errors and poor stopping condition logic in
the list history command.

Fixes google#763
@gdbelvin gdbelvin closed this in bef78fc Aug 18, 2017
@gdbelvin gdbelvin reopened this Aug 18, 2017
@liamsi
Copy link
Contributor Author

liamsi commented Aug 18, 2017

I've updated the kube scripts as well. Will rebase. Then this is ready for another review.

liamsi added 2 commits August 18, 2017 12:33
# Conflicts:
#	cmd/keytransparency-signer/Dockerfile
#	cmd/keytransparency-signer/main.go
#	docker-compose.yml
Copy link
Contributor

@gdbelvin gdbelvin left a comment

Choose a reason for hiding this comment

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

Thanks!

@gdbelvin gdbelvin merged commit eb7ec67 into google:master Aug 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Dockerfiles

3 participants