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

Make it compatible with docker compose 2.x #2236

Merged
merged 3 commits into from
Oct 3, 2021
Merged

Make it compatible with docker compose 2.x #2236

merged 3 commits into from
Oct 3, 2021

Conversation

aminvakil
Copy link
Contributor

Description

docker compose 2.x complains with this error: "key cannot contain a space" or

unexpected character "(" in variable name near "whitelist host after N successful deliveries (N=0 to disable whitelisting)\nPOSTGREY_AUTO_WHITELIST_CLIENTS=5\n\n#

when POSTGREY_TEXT value is something with spaces.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach
Copy link
Member

Hey @aminvakil, thanks for providing a PR :) We aim to maintain DMS for Docker Compose version 3. Therefore, I'm not sure whether we want to have compatibility fixes with Compose version 2. @casperklein @wernerfred your opinion on this?

@aminvakil
Copy link
Contributor Author

I'm not sure if I was clear enough or I'm misunderstanding :)

I'm talking about https://github.com/docker/compose/releases/tag/v2.0.1 which has just been released.

@casperklein
Copy link
Member

I had also issues with the latest docker-compose 2 version and some compose files, but no time yet to take a deeper look.

If white spaces are an issue, we should quote them.

Make it compatible with docker compose 2.x
@aminvakil
Copy link
Contributor Author

If white spaces are an issue, we should quote them.

Thanks, this is definitely a better approach, changed.

@casperklein
Copy link
Member

casperklein commented Oct 3, 2021

The quotes must surround the whole string. Otherwise, the quotes are part of the variable value.

Edit:

https://docs.docker.com/compose/compose-file/compose-file-v3/#env_file

For example if the value is surrounded by quotes (as is often the case of shell variables), the quotes are included in the value passed to Compose.

mailserver.env Outdated Show resolved Hide resolved
@georglauterbach
Copy link
Member

I didn't even know about this - feels like ages since I left Compose :D Nice!

georglauterbach and others added 2 commits October 3, 2021 12:50
Co-authored-by: Casper <casperklein@users.noreply.github.com>
@georglauterbach georglauterbach added area/features kind/improvement Improve an existing feature, configuration file or the documentation orchestrator/compose priority/low labels Oct 3, 2021
@georglauterbach georglauterbach merged commit a83363a into docker-mailserver:master Oct 3, 2021
@casperklein
Copy link
Member

casperklein commented Oct 3, 2021

Damn. I've mistaken it.

You need to surround the whole env string when set directly in docker-compose.yml. However, not when using an extra env_file 😞

@georglauterbach
Copy link
Member

@casperklein just provide a follow up PR:)

@casperklein
Copy link
Member

casperklein commented Oct 3, 2021

That's weird. I will take a deeper look later.

The documentation says, to quote the whole string, even in an env_file. In my test with docker-compose 1.29.2 however, the var is completely ignored, when fully quoted in an env_file.

Two possibilities:

  1. This is a breaking change between the docker-compose versions
  2. The documentation is wrong.

@aminvakil aminvakil deleted the docker_compose_2_env branch October 3, 2021 11:39
casperklein added a commit that referenced this pull request Oct 3, 2021
@casperklein casperklein mentioned this pull request Oct 3, 2021
11 tasks
casperklein added a commit that referenced this pull request Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/features kind/improvement Improve an existing feature, configuration file or the documentation orchestrator/compose priority/low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants