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

docs: improve install/requirements and install/docker-compose #13569

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pyorot
Copy link
Contributor

@pyorot pyorot commented Oct 18, 2024

Giving back changes that mostly would've helped me starting out with Immich:

  1. requirements/software: distinguish Docker Engine and Desktop; clarify that Compose plugin is included with recommended installation guides;
  2. put the hw-accel config file downloads in an optional note;
  3. add a hook after .env file setup suggesting docker-compose.yml edits for optional features;
  4. adding a sentence that "docker compose up -d" takes care of automatic restarts;
  5. linking docker version error note back to "do the recommended install thoroly" with explanation why;
  6. added info on purging obsolete containers;
  7. moved extra install/docker-compose recipe details from quick-start to install/docker-compose.

The idea is to explain some of the basics of Docker (1, 4, 6) and Linux (5) as we go, and streamline out-of-order steps (2, 3, 7). Once I've looked over the compiled docs, I'll set this as ready for review and you can tell me which edits you fancy :).

Copy link
Contributor

github-actions bot commented Oct 18, 2024

Label error. Requires exactly 1 of: changelog:.*. Found: documentation

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 18, 2024
Copy link
Contributor

@mmomjian mmomjian left a comment

Choose a reason for hiding this comment

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

thanks, I have some thoughts

docs/docs/install/docker-compose.mdx Outdated Show resolved Hide resolved
docker compose pull && docker compose up -d
```

To clean up disk space, the old version's obsolete containers can be deleted with the following command, which deletes all stopped containers. As such, make sure to start Immich (with the previous command) before running it.
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
To clean up disk space, the old version's obsolete containers can be deleted with the following command, which deletes all stopped containers. As such, make sure to start Immich (with the previous command) before running it.
To clean up disk space, the old version's obsolete containers can be deleted with the following command, which deletes all unused container images. As such, make sure to start Immich (with the previous command) before running it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn’t aware of images vs containers but looks good, will merge.

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 believe this command can be run with the containers stopped so I deleted that second sentence as well.


:::note
Immich requires the command `docker compose` - the similarly named `docker-compose` is [deprecated](https://docs.docker.com/compose/migrate/) and is no longer compatible with Immich.
Immich requires the command `docker compose` (the plugin); the similarly named `docker-compose` (the standalone) is [deprecated](https://docs.docker.com/compose/migrate/) and is no longer compatible with Immich.
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 this plugin vs standalone talk is more confusing, not less

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For immich’s purposes strictly, it is; however, the Docker Compose installation docs that this page links specifically give the choice between installing Docker Desktop, the Compose plugin, or the Compose standalone.

Copy link
Member

Choose a reason for hiding this comment

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

It gives the choice but also pretty clearly notes that the standalone is deprecated, so I don't think we need to repeat that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s fair yeah; I just figured people might want to join the dots on which installation option corresponds to which command, as an explanation for why the old command doesn’t work. I did delete the reference to “v2” over “v1” since I could hardly find it on Docker’s website. But yeah, omitting those brackets is fine.

- **OS**: Recommended Linux operating system (Ubuntu, Debian, etc).
- Windows is supported with [Docker Desktop on Windows](https://docs.docker.com/desktop/install/windows-install/) or [WSL 2](https://docs.docker.com/desktop/wsl/).
- macOS is supported with [Docker Desktop on Mac](https://docs.docker.com/desktop/install/mac-install/).
- **OS**: Linux is recommended, and Windows and macOS are supported: any version or distribution that Docker can be installed on (see above).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not fond of this change, would prefer it how we had it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate please?

The purpose of this change is to move all Docker variant-selection info into the “Software” section. Not making this change would leave two copies of this info. Arguably, this remaining OS info could entirely be moved into “Software” as well.


```bash title="Start the containers using docker compose command"
docker compose up -d
```

The `restart: always` entries in `docker-compose.yml` for each container mean they will always restart after system reboot or program crashes.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed here (or really at all IMO, this is man docker stuff)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this line is to inform those of us who don’t use or care about Docker that the “compose up” command starts immich as a persistent service, which IMO is a worry that would cross the minds of users who aren’t culturally immersed in Linux to the point of intuitively expecting the behaviour.

The point isn’t to explain “restart: always” but rather to say “this command starts the container and ensures it automatically restarts”. I’ll reword it to make that clearer.

Copy link
Member

Choose a reason for hiding this comment

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

A topic we find ourselves constantly trying to balance is how much basic information to include in our docs. We're documenting Immich, and so I don't think we should expect to be teaching people sysadmin basics. If someone is wondering about details of the restart behaviour, they can find answers to that in plenty of places already.

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 changed it earlier to something closer to what I intended:

This starts immich and ensures it automatically restarts after system reboots or crashes (thanks to the restart: always entries in docker-compose.yml).

The bracketed bit is more for curiosity and can be deleted cos it’s somewhat evident from the non-bracketed bit, if one also reads the yaml file.

But yeah I wouldn’t consider this teaching as much as explaining what that command actually does. On further thought, I recall seeing people forget the d flag, which is kind of a similar mistake – we’re trying to say the command starts the program as a service, so not blocking the console, but as a background thing that auto restarts. Basically that’s what I wanted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New sentence: I am feeling:

This starts immich as a background service, ensuring it restarts after system reboots or crashes.

Or with btw teaching:

This starts immich as a background service (per the -d flag), ensuring it restarts after system reboots or crashes (per the restart fields in docker-compose.yml).

- Populate custom database information if necessary.

:::info Optional Features
You can make the required edits to `docker-compose.yml` to add external libraries or enable hardware acceleration now by following [their guides](#setting-up-optional-features) up to and including those edits.
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
You can make the required edits to `docker-compose.yml` to add external libraries or enable hardware acceleration now by following [their guides](#setting-up-optional-features) up to and including those edits.
You can edit `docker-compose.yml` to add external libraries or enable hardware acceleration now by following [their guides](#setting-up-optional-features).

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 think this is a fair simplification; it’s just annoying that the optional guides don’t split apart the yaml file edits from the settings changes made to the running program, so users who follow this tip would have to flip-flop between the two pages.

Copy link
Contributor

github-actions bot commented Oct 18, 2024

📖 Documentation deployed to pr-13569.preview.immich.app

@pyorot
Copy link
Contributor Author

pyorot commented Oct 18, 2024

Thanks for the review; I'll check it out tomo.

@pyorot
Copy link
Contributor Author

pyorot commented Oct 18, 2024

I flushed all edits and so can open the PR up for formal review.

BTW I force-pushed during the first review so there might be extra changes @mmomjian would like to peep (mostly moving some fine details (e.g. about pwgen) from quick-start into install/docker-compose.

@pyorot pyorot marked this pull request as ready for review October 18, 2024 09:21
@pyorot
Copy link
Contributor Author

pyorot commented Oct 18, 2024

image

Another thing that crossed my mind as a potential change to add is that the order of these features is a bit weird; I'd swap External Libraries and Hardware Transcoding.

@pyorot
Copy link
Contributor Author

pyorot commented Oct 20, 2024

Updated all my suggestions and am happy to stop working on this and allow whichever parts to be merged.

Another thing that crossed my mind as a potential change to add is that the order of these features is a bit weird; I'd swap External Libraries and Hardware Transcoding.

Turns out there’re no sidebar front-matter parameters in the whole features folder so I’ll let a different patch sort that out. The capitalisation of the Supported Formats page’s headings is also inconsistent with the other pages in that folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants