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

Destination aware container names #71

Merged
merged 3 commits into from Mar 9, 2023

Conversation

tbuehlmann
Copy link
Contributor

@tbuehlmann tbuehlmann commented Mar 3, 2023

I started looking into having role aware container names but that turned out to be quite a big undertaking. So I started with destinations instead to see where it could head.

This PR is a start to have destination aware container names so different destinations of the same service can run on the same server.

This doesn't work right now:

# config/deploy.yml
service: app

# config/deploy.staging.yml
web:
  - 1.1.1.1

# config/deploy.demo.yml
web:
  - 1.1.1.1

… as both docker containers would have the same name app-<version> on 1.1.1.1. The PR adds - if present - the destination as part of the container name, ending up as app-staging-<version> and app-demo-<version>.

Mrsk::Commands::App and Mrsk::Commands::Healthcheck should be the only commands that are affected. Accessories might be a candidate, but not sure.

Also, Mrsk::Commands::Healthcheck isn't really affected as the fix port 3999 shouldn't allow parallel deployment of different mrsk apps either way, but it doesn't hurt to have the container name be ready for that in the future.

@dhh: Is this the way we want to pursue with destinations?

@tbuehlmann tbuehlmann marked this pull request as ready for review March 9, 2023 10:55
lib/mrsk/commands/auditor.rb Outdated Show resolved Hide resolved
@dhh dhh merged commit 3026a92 into basecamp:main Mar 9, 2023
@dhh
Copy link
Member

dhh commented Mar 9, 2023

This is great. I'm not sure it's right for accessories to take destinations into considerations. You really want to be explicit with those, since it's usually about stable data.

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

2 participants