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

Determine the compose version via a CLI call and not the docker API. #1021

Merged

Conversation

apollo13
Copy link
Contributor

SUMMARY

Call the CLI to determine the compose version before talking to the API.
Refs #891

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_compose_v2

@apollo13 apollo13 force-pushed the fb-compose-version-via-cli branch 2 times, most recently from 7d075d1 to c8bcafa Compare December 28, 2024 20:55
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Can you please add a changelog fragment? Thanks. (I think this is more like a feature than a bugfix though. The module was never supposed to work with Podman, so it's not a bug that it didn't work with it.)

Regarding the implementation: I think it would be a lot cleaner if you could move the code into multiple methods. One method get_docker_compose_version() (or similar) that calls docker compose version --formta jsonand returns the version as a string, or None if it cannot detemrine the version; and another method get_compose_plugin_version() (or similar) that contains the current code (and returns the version as a string, or fails). And maybe a third method which calls the other two and always returns the version as a string.

plugins/module_utils/compose_v2.py Outdated Show resolved Hide resolved
@apollo13 apollo13 force-pushed the fb-compose-version-via-cli branch from c8bcafa to 3186143 Compare December 29, 2024 10:26
@apollo13
Copy link
Contributor Author

Thanks for your contribution! Can you please add a changelog fragment? Thanks. (I think this is more like a feature than a bugfix though. The module was never supposed to work with Podman, so it's not a bug that it didn't work with it.)

Sure thing, I wanted to know whether it passes CI first :) Added it as minor_change.

Regarding the implementation: I think it would be a lot cleaner if you could move the code into multiple methods.

Done I think, let's see what CI says, locally it did work, but my test env is "limited" to a single python and docker.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've just have suggestion for the changelog fragment. This is about all docker_compose_v2* modules, not just docker_compose_v2 itself.

@apollo13 apollo13 force-pushed the fb-compose-version-via-cli branch from 3186143 to 6a4cc18 Compare December 29, 2024 12:31
@felixfontein felixfontein merged commit 6172a92 into ansible-collections:main Dec 29, 2024
78 checks passed
@felixfontein
Copy link
Collaborator

@apollo13 thanks for your contribution!

@apollo13
Copy link
Contributor Author

Thank you for the quick turnaround and support (as always ;))

@apollo13 apollo13 deleted the fb-compose-version-via-cli branch December 29, 2024 13:14
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