docker_compose - Fix timeout defaulting behavior

[Ajpantuso]

SUMMARY

Removed overwriting of default timeout so stop_grace_period can be correctly read from compose files.
Fixes #25

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

plugins/modules/docker_compose.py

ADDITIONAL INFORMATION

• timeout documentation required default: null since a doc fragment this module extends already defines timeout with default: 60.

After the change the following example from #25 :

 tasks:
     - name: remove pre-existing container to clear logs from previous run
       docker_compose:
         project_src: "{{ project_src }}"
         services:
           - foo
         state: absent

     - name: start service
       docker_compose:
         project_src: "{{ project_src }}"
         services:
           - foo
         state: present

     - name: stop service
       docker_compose:
         project_src: "{{ project_src }}"
         services:
           - foo
         state: present
         stopped: yes

     - command: docker logs applications_foo_1
       register: output

     - debug:
         msg: "{{ output.stdout }}"

results in:

PLAY [localhost] ************************************************************************************************************************************************************************************************

TASK [remove pre-existing container to clear logs from previous run] ********************************************************************************************************************************************
changed: [localhost]

TASK [start service] ********************************************************************************************************************************************************************************************
changed: [localhost]

TASK [stop service] *********************************************************************************************************************************************************************************************
changed: [localhost]

TASK [command] **************************************************************************************************************************************************************************************************
changed: [localhost]

TASK [debug] ****************************************************************************************************************************************************************************************************
ok: [localhost] => {
    "msg": "script has started\nreceived SIGTERM\nexited cleanly"
}

PLAY RECAP ******************************************************************************************************************************************************************************************************
localhost                  : ok=5    changed=4    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

[felixfontein]

Isn't this a breaking change?

[Ajpantuso]

Isn't this a breaking change?

I detailed why it wasn't on the issue thread
Basically the module was enforcing a timeout before compose could apply it's defaulting logic and it was doing this by importing the DEFAULT_TIMEOUT setting from compose. So letting compose handle the defaulting itself results in the same timeout settings. I guess it's only breaking if this module never intended to honor stop_grace_period.

[felixfontein]

Well, it's still a breaking change, since in some cases (user had a different timeout in the service description than the default value) the module will change behavior. I tend to agree with @aaron-kaplan on:

Given that this is long-standing behavior, it might not be a good idea to change the default, but perhaps we could introduce a distinguished timeout value that lets the user explicitly specify that the timeout configured in the project should be used.

I think we should probably keep this for the 2.0.0 release and add a breaking_change changelog fragment. That way, if someone really needs the old behavior, they won't get surprised in a minor release.

[Ajpantuso]

Well, it's still a breaking change, since in some cases (user had a different timeout in the service description than the default value) the module will change behavior. I tend to agree with @aaron-kaplan on:

Given that this is long-standing behavior, it might not be a good idea to change the default, but perhaps we could introduce a distinguished timeout value that lets the user explicitly specify that the timeout configured in the project should be used.

I think we should probably keep this for the 2.0.0 release and add a breaking_change changelog fragment. That way, if someone really needs the old behavior, they won't get surprised in a minor release.

Updated the changelog fragment.
On a side note I was going to implement profiles for docker_compose since that's only a few lines of implementation (and the CLI option is bugged such that it only works when bringing services up so not much to test), but I can hold that for after 1.8.0 if you don't want to wait.

[felixfontein]

Updated the changelog fragment.

Thanks! I guess we'll start with the 2.0.0 preparations somewhen in September; I'll merge this PR then.

On a side note I was going to implement profiles for docker_compose since that's only a few lines of implementation (and the CLI option is bugged such that it only works when bringing services up so not much to test), but I can hold that for after 1.8.0 if you don't want to wait.

Feel free to start working on that now if you want. If the PR is done merged before I do the 1.8.0 release (probably on Tuesday morning), it will be in 1.8.0, otherwise it has to wait for 1.9.0.

[felixfontein]

Restarting tests.

[felixfontein]

@Ajpantuso thanks for contributing this!