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

must warn about unhandled healthcheck command and other options with variables #103

Closed
akostadinov opened this issue Sep 8, 2024 · 6 comments
Labels
duplicate This issue or pull request already exists

Comments

@akostadinov
Copy link

Docker compose seems to replace variables in different commands. This doesn't seem to be the case with systemd.

Take this compose file for example: v1.114.0

Specifically this part:

     healthcheck:
      test: pg_isready --dbname='${DB_DATABASE_NAME}' --username='${DB_USERNAME}' || exit 1; Chksum="$$(psql --dbname='${DB_DATABASE_NAME}' --username='${DB_USERNAME}' --tuples-only --no-align --command='SELECT COALESCE(SUM(checksum_failures), 0) FROM pg_stat_database')"; echo "checksum failure count is $$Chksum"; [ "$$Chksum" = '0' ] || exit 1
      interval: 5m
      start_interval: 30s
      start_period: 5m

The variables DB_* are not replaced (which is good). But they don't work. To make this work I had to remove the quotes and make $ double &&. While keepin gthe other places as they are.

I understand that fixing a random bash command might be impossible. But would be helpful if there is a warning line stating that docker compose would replace these commands before runningthe command in bash while systemd service will not. Maybe some link to an explanation how to fix such commands in general.

Like in this case it is enough to remove the quotes and make double dollar $$. But there might be other considerations when the strings may need quoting.

I think the issue is only with variables that docker-compose would replace. Where shell is supposed to be expanding, original version appears to be fine.

@k9withabone
Copy link
Member

Duplicate of #81?

@akostadinov
Copy link
Author

Might be considered a duplicate. I'm not sure it can fully automatically be resolved.

@k9withabone
Copy link
Member

What do you mean by that?

@akostadinov
Copy link
Author

I mean that in docker compose, these interpolations are performed based on ENV before the command is performed.

While podlet cannot do that. Podlet can only convert these interpolations into bash variables interpolation and hope that nothing will break. And it can break.

Or alternatively podlet can statically replace the strings based on current environment values. In which case it's possible that an environment file changes but service is not regenerated unless systemctl daemon-reload is executed. Maybe this is a better option assuming users will not modify environment files without regenerating the service.

That's why I said the solutions are probably not fully solid.

@k9withabone
Copy link
Member

The first alternative, if I'm understanding correctly, wouldn't work since containers are used in all sorts of ways. Some don't even have a shell installed, like the Podlet container. Also, interpolation can be used for any value in a Compose file, not just commands.

The second alternative is the current plan. I don't really see how else it could be done. However, as always, I'm open to other ideas. You should know that Podlet is not run as part of systemctl daemon-reload. If you change the .env file, you will have to run Podlet again to recreate the Quadlet files (or change the value in the Quadlet file directly), then systemctl daemon-reload to regenerate the service files.

Further discussion about how interpolation should be implemented should be done in #81.

@k9withabone k9withabone closed this as not planned Won't fix, can't repro, duplicate, stale Sep 20, 2024
@k9withabone k9withabone added the duplicate This issue or pull request already exists label Sep 20, 2024
@akostadinov
Copy link
Author

Yeah, I knew it can't automatically work so a warning will be needed. But forgot why. So to me important is to have a warning and explaining the approaches. In my particular case, it made more sense to make it bash interpolation instead.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants