Skip to content
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

WIP: Dockerize #39

Merged
merged 9 commits into from
Jan 22, 2020
Merged

WIP: Dockerize #39

merged 9 commits into from
Jan 22, 2020

Conversation

ethanfrey
Copy link
Member

Closes #38

Check README for how to run local Docker image for testing

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Reviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge PR #XYZ: [title]" (coding standards)

@ethanfrey ethanfrey requested a review from alpe January 21, 2020 18:29
@codecov-io
Copy link

codecov-io commented Jan 21, 2020

Codecov Report

Merging #39 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
- Coverage   61.64%   61.46%   -0.19%     
==========================================
  Files          15       15              
  Lines        1095     1095              
==========================================
- Hits          675      673       -2     
- Misses        354      356       +2     
  Partials       66       66
Impacted Files Coverage Δ
lcd_test/helpers.go 75.53% <0%> (-0.72%) ⬇️

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 b5f34a7...0380ead. Read the comment docs.

@ethanfrey
Copy link
Member Author

@alpe I thought last night and there are at least 2 missing features here:

  1. The setup.sh should take a list of addresses. The script can then loop through $@ and run add-genesis-account for each of those. They can all have the same amount.
  2. There should be two forms to run. One (current run.sh) assumes we mount an initialized directory (from setup.sh or a previous run) and just write to it. The other is designed for CI. setup.sh can write to one volume under /root. Then for run, we mount to eg. /template and the other run_ci.sh script can copy all files from /template/.wasm{d,cli} into /root/* (which is in the docker tempfs) and then start the normal run script. This lets us initialize setup once (which requires interactive password entering), and then use that initialized data for CI runs without overwriting it.

If you can think of anything else useful for local testing/CI, please add that as well.

@alpe
Copy link
Contributor

alpe commented Jan 22, 2020

Very nice to see this working now! 🎉

  1. The setup.sh should take a list of addresses. The script can then loop through $@ and run add-genesis-account for each of those. They can all have the same amount.

This would not be hard to implement but maybe hard to communicate. Instead of running a simple script people would need to think about arguments now and remember them for later use. My second note is about the password entry. It is already not very satisfying to run the script for generating a single key. If we can use a default password that's not an issue.

  1. There should be two forms to run. One (current run.sh) assumes we mount an initialized directory (from setup.sh or a previous run) and just write to it. The other is designed for CI. setup.sh can write ...

Not sure if I understood it correct. For CI we would need some defined state. IMHO this can be achieved best by mounting predefined .wasm{d,cli}/ folders from the repo into the container under /root. No setup.sh execution during CI.

Dockerfile_demo Outdated
@@ -0,0 +1,30 @@
# Simple usage with a mounted data directory:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's delete this file

Copy link
Contributor

Choose a reason for hiding this comment

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

and the the docker_demo.md

@@ -2,32 +2,31 @@
# > docker build -t gaia .
# > docker run -it -p 46657:46657 -p 46656:46656 -v ~/.wasmd:/root/.wasmd -v ~/.wasmcli:/root/.wasmcli gaia wasmd init
# > docker run -it -p 46657:46657 -p 46656:46656 -v ~/.wasmd:/root/.wasmd -v ~/.wasmcli:/root/.wasmcli gaia wasmd start
FROM golang:alpine AS build-env
FROM golang:1.13-buster AS build-env
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments above are outdated. Proper ones are in the README. let's remove them here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let's remove all docs except readme

# tendermint p2p
EXPOSE 26656
# tendermint rpc
EXPOSE 26657
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good for doc

@ethanfrey
Copy link
Member Author

Not sure if I understood it correct. For CI we would need some defined state. IMHO this can be achieved best by mounting predefined .wasm{d,cli}/ folders from the repo into the container under /root. No setup.sh execution during CI.

My idea is I, dev, can run setup.sh to make an initial CI state. And I save that to disk/commit to git repo. Thus, I also want setup to be easy to use for such cases.

In the CI I only use run with the predefined state and it doesn't need a password

@ethanfrey
Copy link
Member Author

My second note is about the password entry. It is already not very satisfying to run the script for generating a single key. If we can use a default password that's not an issue.

If you can get the script to run non interactive without password entry, even default password, that would be great. Maybe another name to leave both options?

There was just a PR #40 about using Keyring headless, maybe that has some useful tips

@ethanfrey ethanfrey merged commit 2347ae4 into master Jan 22, 2020
@ethanfrey ethanfrey deleted the alex/docker_wip branch January 22, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create working docker image for CI/testing
3 participants