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

Add docker compose v2 module #540

Closed
wants to merge 3 commits into from

Conversation

Sid-Sun
Copy link
Contributor

@Sid-Sun Sid-Sun commented Dec 29, 2022

Signed-off-by: Sid Sun [email protected]

SUMMARY

Add module to support docker compose v2 via python-on-whales
the docker_compose_v2 module does not behave the same as the existing module, it lacks some features and does some things differently
returns from module are somewhat finicky as python on whales has a slightly weird logic to it

adds lacking support for compose v2 #216

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

docker_compose_v2

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! I've added some first comments. Please also observe the failing tests.

plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
plugins/modules/docker_compose_v2.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

@Sid-Sun I pushed some basic tests for the module to your branch. I've added the setup role in another PR, so if you want to run the tests locally, you first have to rebase your branch against the current main branch.

To run the tests directly, you can add a playbook tests/integration/targets/docker_compose_v2/run-tests-locally.yml with the following content:

- hosts: localhost
  tasks:
  - name: Check Docker API version
    command: "{{ ansible_python.executable }} -c 'import docker; print(docker.from_env().version()[\"ApiVersion\"])'"
    register: docker_api_version_stdout
    ignore_errors: yes

  - set_fact:
      docker_api_version: "{{ docker_api_version_stdout.stdout or '0.0' }}"
      remote_tmp_dir: "{{ 'output-%0x' % ((2**32) | random) }}"
      has_docker_compose: true

  - debug:
      msg: "Docker API version: {{ docker_api_version }}"

  - include_vars: ../setup_docker/vars/main.yml

  - block:
    - name: Create output directory
      file:
        path: "{{ remote_tmp_dir }}"
        state: directory
    - include_tasks: tasks/main.yml
      vars:
        role_path: '.'
    always:
    - name: Remove output directory
      file:
        path: "{{ remote_tmp_dir }}"
        state: absent

and run ansible-playbook run-tests-locally.yml in that directory. Note that the playbook uses your local Docker Daemon; it tries to clean up, but if the module is broken you might have some leftovers. (Right now cleanup works.)

(When you run the tests you will notice that some of the tests are currently failing, for example check mode and idempotency.)

@gotmax23
Copy link
Contributor

gotmax23 commented Dec 30, 2022

python-on-whales only supports Python 3.7 and above. Shouldn't the minimum supported Python version be mentioned in the module docs and the README?

@felixfontein felixfontein added the docker-compose-v1 Docker Compose v1 label Jan 6, 2023
@github-actions
Copy link

github-actions bot commented Jan 15, 2023

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.

@jkossis
Copy link

jkossis commented Jan 16, 2023

I know this is still a WIP, but I was curious if the plan is for this implementation to reflect the idempotent behavior of v1.

For example, in v1, pulling an image is only marked as changed when the image is in fact new/different. Whereas here, it's marked as changed as long as we aren't running under check_mode. This could be extended to other operations, just used pull as an example.

@Sid-Sun
Copy link
Contributor Author

Sid-Sun commented Jan 24, 2023

thanks for the tests @felixfontein idempotency requires getting output from python-on-wheels and the current logic in POW doesn't allow for returning output, i'll try reworking it

@felixfontein
Copy link
Collaborator

Idempotency is a basic feature that almost all Ansible modules have (and should have). The exceptions are mainly cases where idempotency is impossible, for example the ansible.builtin.command module, since that module cannot know whether a random command will result in a change or not, or APIs that do not allow to query certain information (for security reasons, because it is computationally infeasible, ...), etc. I think that for most cases it should be possible to implement idempotency for this module (pulling images is one of the cases where it is not so easy to implement, and probably impossible in case of check mode).

@lel-amri
Copy link
Contributor

Hello there,

I just pulled code today and noticed the setup_docker_compose_v2 test files, indicating that work was ongoing for supporting docker-compose v2.

I am currently working on this as well, although I took the module.run_command() approach as "hinted" in this comment. I took inspiration in how POW handles docker-compose and how the containers.podman collection handles podman kube play.

I believe we should let docker-compose manage indempotency all by itself unless we want to handle potentially clashing behavior. Getting the result of a docker-composer run is definitely achievable by reading what is written on stdout (while using the --ansi never option), but we have to keep in mind that output stability is not guaranteed. Check mode, though, seems impossible.

Now, about my ongoing work, I just want to get the job done and I don't see the point of both of us working on different implementations. I'm uneasy because I'd like to join the effort of supporting docker-compose v2, but after working on this I believe that the module.run_command() way is better than the POW way for two reasons : there is less dependencies and POW does not returns the output nor parses what docker-compose spits out. What do you think ?

@Sid-Sun
Copy link
Contributor Author

Sid-Sun commented Feb 17, 2023

I agree,we should let docker compose manage idempotency.

POW for this usecase seems to be a bit unideal, I have an open PR to change the API of POW to at least return the output though it hasn't gone anywhere yet.

Thanks for working on this! hoping to see your implementation merge!😀

@Sid-Sun
Copy link
Contributor Author

Sid-Sun commented Feb 17, 2023

I'll close this PR for now.

@Sid-Sun Sid-Sun closed this Feb 17, 2023
@felixfontein
Copy link
Collaborator

@Sid-Sun thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker-compose-v2 Docker Compose v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants