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 information about docker-compose.yml to README.md #4243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lumpsoid
Copy link

@lumpsoid lumpsoid commented Jan 14, 2024

  • added information on how this can be organized into a docker-compose.yml file for easier installation.
  • added a couple of examples of what happens in docker-compose.yml
  • slightly reshaped the text in #Instalation
  • add the link to the wiki page "Using Docker Compose"

because of the cleaning of this branch, github closed previous pull request, sorry

add the yaml block with the simple docker compose set up with the pinned docker image.
add the link to the wiki page "Using Docker Compose".

the left side of `volumes:` is where you can find vaultwarden data on your PC/server relative to the folder where you store the docker-compose.yml file
example:
```yaml
Copy link

Choose a reason for hiding this comment

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

This is not yaml. Could maybe use text instead?

Copy link

Choose a reason for hiding this comment

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

Adding context (response to the comment that was removed). Just line 62 using yaml does not seem correct to me as the contents of the code fence is a directory structure example, not actual yaml. The yaml above for the compose is accurately using yaml for its code fence.

- 80:80
restart: unless-stopped
```
and `docker compose up` or `docker compose up -d` to pull and run conteiner.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix spelling of container instead of conteiner

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

The previous PR had several comments, which do not seem to be addressed here.
Please make sure it is up-to-date with the comments there.

Copy link
Contributor

@stefan0xC stefan0xC left a comment

Choose a reason for hiding this comment

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

I'm not sure if the README is the best place for a docker-compose.yml example. Overall I think that the whole sections needs to be rewritten.

services:
vaultwarden:
container_name: vaultwarden
image: vaultwarden/server:1.30.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
image: vaultwarden/server:1.30.0
image: vaultwarden/server:latest

<vaultwarden data from the /data folder inside the container>
```

the left side of `ports:` is the port your instance will run on (example: `http://127.0.0.1:80`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Your example is a bit confusing because by default 80:80 will bind to 0.0.0.0 (meaning all network interfaces) and not 127.0.0.1 which might be important (because to achieve that you have to be specific, i.e. 127.0.0.1:80:80). However, I don't think that our README is the right place for explaining the intricacies of networking in compose.

volumes:
- ./vw-data/:/data/
ports:
- 80:80
Copy link
Contributor

@stefan0xC stefan0xC Jan 29, 2024

Choose a reason for hiding this comment

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

Suggested change
- 80:80
- "80:80"

Docker's recommendation is "always explicitly specifying your port mappings as strings".

```
and `docker compose up` or `docker compose up -d` to pull and run conteiner.

the left side of `volumes:` is where you can find vaultwarden data on your PC/server relative to the folder where you store the docker-compose.yml file
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this explanation I think it's better to link to https://docs.docker.com/storage/ (which I would do above in a general section about persistent storage because most of it would also apply to the docker run example) and just refer to the wiki for further information.

@@ -35,13 +35,41 @@ Basically full implementation of Bitwarden API is provided including:

## Installation
Pull the docker image and mount a volume from the host for persistent storage:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you need to rewrite the Installation section as it's currently interrupted by the added subsections, especially because the existing documentation only perfunctorily explains the docker commands.

@BlackDex
Copy link
Collaborator

I actually think we should create a better Wiki page on how to use it, and link that in the Readme.
The main reason for this is, what about podman, or other container runtimes?

That would only clutter the whole Readme while that is better suited for a good written Wiki page.
Then we can still mention using docker, docker compose or podman etc.. in the readme and link to that article.

I'm leaning to closing this PR because of this. If you can create a good wiki article, or update an existing one and link to it in this PR then that is also fine :).

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.

None yet

5 participants