Skip to content

docker: provide default container names#12962

Closed
dominic-r wants to merge 1 commit intogoauthentik:mainfrom
dominic-r:sdko/docker/provide-default-container-names
Closed

docker: provide default container names#12962
dominic-r wants to merge 1 commit intogoauthentik:mainfrom
dominic-r:sdko/docker/provide-default-container-names

Conversation

@dominic-r
Copy link
Member

What?

This PR adds default container names to the Postgres database, Redis, server, and worker.

The majority of Docker Compose files (for authentik and other services out there in the internet) already hard-code container names, so this change aligns with common best practices.

By setting explicit names, we allow users to reference containers directly with commands like:

docker exec -it authentik-worker ak

instead of relying on docker compose [...], which fails with

no configuration file provided: not found

if the user is not in the directory containing the Compose file.

This improves the experience of the user (and mine too as I need to reference these containers in some doc pages) when following documentation steps and makes the output of docker ps less of a mess.

Details

REPLACE ME


Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If an API change has been made

  • The API schema has been updated (make gen-build)

If changes to the frontend have been made

  • The code has been formatted (make web)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

Signed-off-by: Dominic R <dominic@sdko.org>
@dominic-r dominic-r requested a review from a team as a code owner February 8, 2025 17:31
@netlify
Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit 16b4da2
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/67a794ed185a2e00087ac38a
😎 Deploy Preview https://deploy-preview-12962--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for authentik-docs ready!

Name Link
🔨 Latest commit 16b4da2
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/67a794ed91eab50008eb64ae
😎 Deploy Preview https://deploy-preview-12962--authentik-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dominic-r
Copy link
Member Author

dominic-r commented Feb 8, 2025

I presume, if this is accepted, I'll also have to add it to the 2025.x.md/next.md/whatever it was next-release release notes?

@codecov
Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.69%. Comparing base (daebeb1) to head (16b4da2).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12962      +/-   ##
==========================================
- Coverage   92.75%   92.69%   -0.07%     
==========================================
  Files         785      785              
  Lines       39580    39580              
==========================================
- Hits        36714    36689      -25     
- Misses       2866     2891      +25     
Flag Coverage Δ
e2e 48.42% <ø> (-0.14%) ⬇️
integration 24.54% <ø> (ø)
unit 90.43% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rissson
Copy link
Member

rissson commented Feb 8, 2025

While I'm not necessarily against this change, this might (?) be a breaking change. I'm not sure overriding the way docker-compose manages container names is something we want to do. What were the specific reasons for this change?

@dominic-r
Copy link
Member Author

dominic-r commented Feb 8, 2025

While I'm not necessarily against this change, this might (?) be a breaking change. I'm not sure overriding the way docker-compose manages container names is something we want to do. What were the specific reasons for this change?

Won't really be a breaking change. Users will rebuild containers and use the --remove-orphans option to remove old containers. Everything else should work just fine. As for specific reasons, there's nothing that can't be done if container_name is not set. It's often shorter than the name Docker provides, easier for quick docker logs -f (without being in compose directory), things like that. This is just a proposition TBH That and the reasons in PR description

@rissson
Copy link
Member

rissson commented Feb 10, 2025

Won't really be a breaking change. Users will rebuild containers and use the --remove-orphans option to remove old containers

That's exactly where the break happens. For one thing, users with automated deploy processes (even though I highly doubt they just download the docker-compose in that case) might not remove orphan containers by default. Also, we don't include that option in our upgrading docs.

@dominic-r
Copy link
Member Author

Considering what you mentioned and the false motivation related to the backup PR, what do you suggest doing with this one? Closing it?

@rissson
Copy link
Member

rissson commented Feb 10, 2025

Yeah I don't think we need/want this change for now

@dominic-r dominic-r closed this Feb 10, 2025
@dominic-r dominic-r deleted the sdko/docker/provide-default-container-names branch February 10, 2025 15:41
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.

2 participants