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

Update README.md with Podman instructions #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rmonk
Copy link

@rmonk rmonk commented Jun 26, 2023

Add systemd/podman configuration to configure the container similar to the watchtower configuration for Docker. This should satisfy #75

Add systemd/podman configuration to configure the container similar to the watchtowerr configuration for Docker.
Copy link

@rewbycraft rewbycraft left a comment

Choose a reason for hiding this comment

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

Generally looks nice. I've left some feedback.

As a side note, it would maybe also be interesting to add a similar guide to https://github.com/ArchiveTeam/standalone-readme-template or the wiki or something for standalone project containers.

Podman Compose works almost identically to Docker's version but watchtower is not the recommended auto-update method. To achive a similar effect:
#### One Time Setup
1. (as root) Set up the base systemd unit file for containers:
`sudo /usr/local/bin/podman-compose systemd --action create-unit`

Choose a reason for hiding this comment

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

This line assumes where podman-compose is installed. We should rely on $PATH here.


### Podman Compose

Podman Compose works almost identically to Docker's version but watchtower is not the recommended auto-update method. To achive a similar effect:

Choose a reason for hiding this comment

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

This guide should mention that this is for (what appears) to be a rootless podman setup which may not be what the user has installed.

If it's not too much effort, it would be nice if this guide would either have two separate guides, one for rootless and one for root. Or at least mention the "do this if rootless, this if not".

`mkdir -p ~/containers`

#### For each container:
1. Set up a directory to hold the container and other files:

Choose a reason for hiding this comment

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

It's been a while since I've done podman, I admit. But why do they need to do this for "each container"?
If I recall correctly, podman-compose supports the scale parameter.

#### For each container:
1. Set up a directory to hold the container and other files:
`mkdir -p ~/containers/archiveteam; cd ~/containers/archiveteam`
2. Create `container-compose.yml` in the new directory:

Choose a reason for hiding this comment

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

Rather than have the user copy paste this file, why not include it in the repo as a dedicated file much like the docker-compose file is included.

HTTP_PASSWORD=
SELECTED_PROJECT=auto
CONCURRENT_ITEMS=
```

Choose a reason for hiding this comment

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

I have some thoughts in this.
Only DOWNLOADER and SELECTED_PROJECT should be specified (with project defaulting to auto as it does here) in order to be in line with the docker instructions. (Although I could be convinced to add the HTTP_ ones in as well as long as there is an explicit instruction to actually put some sensible values in them.)

Concurrency is something that can be rather sensitive; people setting it too high often causes 429 issues.
As such, I would argue that it's best left out. Those who know what it means and care are also smart enough to read the part of the readme where it mentions the var.

services:
archiveteam:
image: atdr.meo.ws/archiveteam/warrior-dockerfile
container_name: archiveteam

Choose a reason for hiding this comment

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

I wouldn't set a container name to be honest. This way people who want to run multiples can use scale

@Arkiver2
Copy link
Member

Thank you for the PR! It looks pretty good and if the comments/issues from @rewbycraft have been addressed we can merge this.

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

3 participants