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

Feature/docker #785

Closed
wants to merge 12 commits into from
Closed

Feature/docker #785

wants to merge 12 commits into from

Conversation

dasrecht
Copy link

Hi there 👋

As discussed during the chaos communication congress I used docker to deploy an ooni-probe.

I've added a bit of documentation around it, the only thing left would be adding this repository to the docker hub on the openobservatory organisation https://hub.docker.com/u/openobservatory/

The way I currently run will not persist any data on the host so every container start will start from scratch.

/b

@coveralls
Copy link

coveralls commented Dec 29, 2017

Coverage Status

Coverage decreased (-0.05%) to 71.085% when pulling d560005 on dasrecht:feature/docker into 625268f on TheTorProject:master.

@coveralls
Copy link

coveralls commented Dec 29, 2017

Coverage Status

Coverage decreased (-6.6%) to 64.488% when pulling 5cd773b on dasrecht:feature/docker into 625268f on TheTorProject:master.

@coveralls
Copy link

coveralls commented Dec 29, 2017

Coverage Status

Coverage decreased (-6.6%) to 64.488% when pulling 5cd773b on dasrecht:feature/docker into 625268f on TheTorProject:master.

@hellais
Copy link
Member

hellais commented Dec 30, 2017

Re-starting travis to see if now that the klein patch is merged, the CI will succeed.

.coveragerc Outdated
@@ -1,4 +1,4 @@
[run]
source = ooni
[report]
omit = ooni/nettests/*, ooni/api/*, ooni/kit/*, ooni/contrib/*
omit = ooni/nettests/*, ooni/api/*, ooni/kit/*, ooni/contrib/* Dockerfile docker-compose.yml README.rst
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need to add text files to test .coveragerc. Since those are not source files, it will already be ignored by coverage.py.

Copy link
Author

Choose a reason for hiding this comment

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

makes sense. will add a commit for that

README.rst Outdated
@@ -91,6 +91,28 @@ that you have the ``universe`` repository enabled. The ``universe`` repository
is enabled by default in a standard Ubuntu installation but may not be on some
minimal, or not standard, installations.

On Docker::
Copy link
Member

Choose a reason for hiding this comment

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

I think you have an extra : in here.

README.rst Outdated
- Docker
- docker-compose

You can pull the image from our Docker Hub
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to put at the end of the line :: so that it considers this a code block (as opposed to inline code)

README.rst Outdated
docker run -p 8842:8842 -d openobservatory/ooni-probe:latest

If you prefer to run the ooni-probe in a more persistent manner on your host
you can add `--restart always` to the execution.
Copy link
Member

Choose a reason for hiding this comment

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

Here it's the same as above.

README.rst Outdated
docker run -p 8842:8842 --restart always -d openobservatory/ooni-probe:latest

On the other hand you can also build the image on your local machine:

Copy link
Member

Choose a reason for hiding this comment

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

Here you are missing an extra :

@@ -0,0 +1,7 @@
version: '2'
Copy link
Member

Choose a reason for hiding this comment

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

I think given that it's just a single service we don't really need a docker-compose.yml file. We can just use plain docker or wrap the commands above in a shell script.

README.rst Outdated

git clone https://github.com/TheTorProject/ooni-probe.git
docker-compose build
docker-compose up
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace these two commands with just vanilla docker (without docker-compose) and remove the docker-compose dependency in the above lines?

Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

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

I would suggest we drop docker-compose as a dependency and left some other comments for nit-picks to fix before merging.

Thanks for hacking on this!

@coveralls
Copy link

coveralls commented Dec 30, 2017

Coverage Status

Coverage decreased (-6.7%) to 64.53% when pulling d456c3f on dasrecht:feature/docker into 18a818b on TheTorProject:master.

@dasrecht
Copy link
Author

dasrecht commented Dec 30, 2017

@hellais thanks for the review, updated as per your comments :)

TIL: writing reStructuredText - I'm way to used to markdown at the moment.

@coveralls
Copy link

coveralls commented Dec 30, 2017

Coverage Status

Coverage decreased (-0.05%) to 71.134% when pulling e653d2e on dasrecht:feature/docker into 18a818b on TheTorProject:master.

@coveralls
Copy link

coveralls commented Jan 9, 2018

Coverage Status

Coverage decreased (-0.05%) to 71.134% when pulling 0849243 on dasrecht:feature/docker into 2b06bcc on TheTorProject:master.

@hellais
Copy link
Member

hellais commented Sep 13, 2018

Issue moved to ooni/probe-legacy #16 via ZenHub

@hellais hellais closed this Sep 13, 2018
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.

4 participants