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

Add detail about membership of the docker group in Ubuntu/Debian quickstart. #3815

Merged
merged 2 commits into from
Sep 27, 2018

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Sep 11, 2018

Status

Ready for review.

Description of Changes

A minor documentation edit so docker related installation / setup has complete steps. Will save someone, somewhere 15 minutes of Google-ing.

Testing

Build the docs and visit /development/setup_development.html to check.

Deployment

Any special considerations for deployment?

None that I know of.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

@@ -39,6 +39,11 @@ be installed via the official documentation links:
.. _`Docker CE for Ubuntu`: https://docs.docker.com/install/linux/docker-ce/ubuntu/
.. _`Docker CE for Debian`: https://docs.docker.com/install/linux/docker-ce/debian/

.. warning::

The official documentation currently misses the detail that the user needs
Copy link
Contributor

Choose a reason for hiding this comment

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

By "official documentation" do you mean SecureDrop's or Docker's? Additionally, all commands can be sudo'd which is what Docker recommends since having a user in the docker group is actually a security hole as it effectively means commands can be run as root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha... I meant dockers docs.

I didn't see the instructions for using sudo in their docs and the example commands I read in both docker and SecureDrop suggest (to me) that sudo wasn't expected, hence this change. However, please disregard this change given the security concerns of being in the docker group. In any case, would be good to know what advice to give (I was just logging this as I worked my way through the instructions). Thanks!


The official documentation currently misses the detail that the user needs
to be in the ``docker`` group. This is easily achieved with
``sudo adduser $USER docker`` and restarting your session.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this be sudo adduser $USER -G docker ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so... (from adduser --help):

adduser USER GROUP
   Add an existing user to an existing group

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not the case for Fedora/CentOS/RHEL world. This command will not work :)

Copy link
Contributor

Choose a reason for hiding this comment

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

But, this is for Ubuntu/Debian, so skip this :)

@conorsch
Copy link
Contributor

Rather than include steps ourselves, we should link out to upstream docs wherever possible, to reduce maintenance burden on ourselves. I suggest the following patch in lieu of the "warning" box:

diff --git a/docs/development/setup_development.rst b/docs/development/setup_development.rst
index 27a4d5fb1..66214cd0a 100644
--- a/docs/development/setup_development.rst
+++ b/docs/development/setup_development.rst
@@ -36,8 +36,11 @@ be installed via the official documentation links:
 * `Docker CE for Ubuntu`_
 * `Docker CE for Debian`_
 
+Make sure to follow the `Post-installation steps for Linux`_, as well.
+
 .. _`Docker CE for Ubuntu`: https://docs.docker.com/install/linux/docker-ce/ubuntu/
 .. _`Docker CE for Debian`: https://docs.docker.com/install/linux/docker-ce/debian/
+.. _`Post-installation steps for Linux`: https://docs.docker.com/install/linux/linux-postinstall/
 
 
 macOS

Can you give that a shot, @ntoll, and see if it clarifies the setup for you?

@conorsch conorsch self-requested a review September 11, 2018 17:31
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Suggest changing the warning box to a link out to the group mgmt docs in upstream official Docker docs.

@ntoll
Copy link
Contributor Author

ntoll commented Sep 12, 2018

+1 Fixing now.

@redshiftzero
Copy link
Contributor

good to go @conorsch?

@redshiftzero redshiftzero dismissed conorsch’s stale review September 27, 2018 22:16

conor's comment was specific and addressed by the PR author

@redshiftzero redshiftzero merged commit bb17761 into freedomofpress:develop Sep 27, 2018
@redshiftzero
Copy link
Contributor

thanks @ntoll!

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.

5 participants