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

remove container_name? #88

Merged
merged 2 commits into from
Oct 5, 2023
Merged

remove container_name? #88

merged 2 commits into from
Oct 5, 2023

Conversation

wardi
Copy link
Contributor

@wardi wardi commented Sep 26, 2023

For discussion: Removing the container_name lines lets us run many ckan-docker sites at the same time (assuming you use different ports)

What is the benefit in forcing a container name in the docker compose config?

For discussion: Removing the `container_name` lines lets us run many ckan-docker sites at the same time (assuming you use different ports)

What is the benefit in forcing a container name in the docker compose config?
@kowh-ai
Copy link
Contributor

kowh-ai commented Sep 28, 2023

The CKAN Docker install has always had container_name: in the compose file. Previously to the current implementation the names were hard coded. Now they are variables referenced from the .env file. I guess it’s done for container name consistency ie: referencing a container from within the container network, perhaps for monitoring, although we don’t offer that…Would you want to build/run the same container under different service names within the same compose file?

@wardi
Copy link
Contributor Author

wardi commented Sep 28, 2023

my use case is cloning this repo into different directories for different versions of ckan, test environments and work on different extensions. With container_name set this way I either need to customize all the container names in every .env file or I can only run one docker compose environment at a time on my machine.

My fix has been to comment out the container_name lines and let docker compose automatically assign them names based on the directory name plus an index.

@kowh-ai
Copy link
Contributor

kowh-ai commented Oct 4, 2023

@wardi - Do you have to update the .env file at all after cloning to a new directory? If so, why not add another environment variable to .env, say ENV and set this to the environment name ie: dev3 and then use a docker-compose.yml file that has modifed the container_name: to be like the following:
...
nginx:
container_name: ${ENV}-${NGINX_CONTAINER_NAME}
build:
...

You would need to make one change to the .env file per new environment

@wardi
Copy link
Contributor Author

wardi commented Oct 4, 2023

My point is that if you don't have container name set in the yaml file then docker compose does this automatically based on the directory name (it even adds a suffix to avoid conflicts IIUC)

Unless there's something I'm missing removing these names would be less effort, more standard and more flexible than adding a new .env setting.

@kowh-ai
Copy link
Contributor

kowh-ai commented Oct 5, 2023

@wardi - OK, sure I'll merge this...thanks.

I'll update the README to mention adding container_name: to the compose yaml files if there is a requirement for consistent container names for whatever reason. I'll also remove the *_CONTAINER_NAMES from the .env.example file and make similar changes to the docker-compose.dev.yml file in a separate PR for consistency

@wardi
Copy link
Contributor Author

wardi commented Oct 5, 2023

@kowh-ai thanks, I think you can update this PR if you prefer because it's part of this repo

@kowh-ai kowh-ai merged commit 4469079 into master Oct 5, 2023
1 check passed
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