docker_swarm_service: Remove defaults

[hannseman]

SUMMARY

This PR removes all argument defaults. It will only send required parameters to docker. This fixes an issue where the current module arguments have defaults that are not compatible with the specified docker-py minimum version. Today the minimum docker-py version is 3.0.0 when the stated one is 2.0.0.

This PR also includes a couple of documentation related fixes:

• The yaml in the module documentation and examples have been indented and linted with yamllint.
• The module examples have been updated. They were quite confusing and more looked like copy pasted test cases.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

docker_swarm_service

[ansibot]

cc @DBendit @akshay196 @danihodovic @dariko @felixfontein @jwitko @kassiansun @tbouvet
click here for bot help

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:437:24: W291 trailing whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:442:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:1: W293 blank line contains whitespace
lib/ansible/modules/cloud/docker/docker_swarm_service.py:474:1: W293 blank line contains whitespace

click here for bot help

[hannseman]

@felixfontein @dariko Looks like we may even have to increase the docker api minimum version to 1.32. https://github.com/docker/docker-py/blob/master/docker/api/service.py#L40

It shouldn't be possible for anyone to have used this module successfully with docker-py less than 3.0.0 or docker api less than 1.32. Please correct me if I'm wrong.

[hannseman]

The CI failures are caused by the unstable setup_docker on fedora: https://app.shippable.com/github/ansible/ansible/runs/103417/51/tests

[felixfontein]

I guess a better solution would be to make sure that the module also works with older docker-py and docker API versions.

The best way to achieve this is probably to change generate_docker_py_service_description so that it returns a dict

return {
    update_policy=update_policy,
    task_template=task_template,
    networks=networks,
    endpoint_spec=endpoint_spec,
    mode=mode,
    labels=self.labels,
}

instead of a tuple, and the calling functions update_service and create_service include the result in the client calls via **result. Then one can modify the generate_docker_py_service_description function to only include keys in the dict which are actually needed.

[hannseman]

That’s true and should be an easy fix. Would have been nice to jump up a major version on docker-py but I guess there’s a lot of people stuck on older ones.

We’ll have to limit publish and endpoint_mode to docker py 3.0.0. Since they’re defined in EndpointSpec.

This fix would depend on #51232 since that removes the default vip for endpoint_mode.

[hannseman]

Found two more options with defaults that would need to be dropped to maintain the requirement of docker-py 2.0.0.

• update_monitor has the default 5000000000. The default in docker is also 5s so should be non-breaking to drop it. https://docs.docker.com/engine/reference/commandline/service_create/
• update_max_failure_ratio has the default 0.00. The default in docker is 0 so it should also be non-breaking to drop it.

Added in 2.1.0 https://github.com/docker/docker-py/blob/master/docs/change-log.md#210 and API version 1.25.

https://github.com/docker/docker-py/blob/master/docker/api/service.py#L16

[hannseman]

secrets also has a default as [] in the argument_spec. It would be best if that default was made to be None to be able to exclude it from the arguments to types.ContainerSpec by a None-check.

hostname has the default of "". Would be nicer if it too was set to None.

container_spec_args = {
    'image': self.image,
    'command': self.command,
    'args': self.args,
    'env': self.env,
    'user': self.user,
    'labels': self.container_labels,
    'mounts': self.mounts
}
if self.tty is not None:
    container_spec_args['tty'] = self.tty
if secrets is not None:
    container_spec_args['secrets'] = secrets
if dns_config is not None:
    container_spec_args['dns_config'] = dns_config
if configs is not None:
    container_spec_args['configs'] = configs
if hostname is not None:
    container_spec_args['hostname'] = hostname

cspec = types.ContainerSpec(**container_spec_args)

[hannseman]

@felixfontein this PR turned out to be a bit bigger than expected. There was a lot of arguments that was not compatible with docker-py 2.0.0 but had defaults. After a while there wasn't much left so I felt a bit bold and removed all defaults as they all mapped to existing docker defaults.

All non-required docker arguments are now not passed to docker if not explicitly specified. The endpoint_mode default is left since that is handled by #51232.

I'm aware that removing defaults is not something that is made easy but in my opinion it's better to try and fix this now than later.

[ansibot]

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:750:40: bad-whitespace Exactly one space required before comparison             if self.update_parallelism  == 2:                                         ^^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:750:39: E221 multiple spaces before operator

The test ansible-test sanity --test validate-modules [explain] failed with 6 errors:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "log_driver" does not match the documentation ('json-file')
lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "restart_policy" does not match the documentation ('none')
lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "update_delay" does not match the documentation (10)
lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "update_failure_action" does not match the documentation ('continue')
lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "update_monitor" does not match the documentation (5000000000)
lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E324 Value for "default" from the argument_spec (None) for "update_parallelism" does not match the documentation (1)

click here for bot help

[felixfontein]

test

[felixfontein]

bot_status

[ansibot]

Components

lib/ansible/modules/cloud/docker/docker_swarm_service.py
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

test/integration/targets/docker_swarm_service/tasks/tests/options.yml
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

test/integration/targets/docker_swarm_service/vars/main.yml
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 1
shipit_actors (maintainers or core team members): felixfontein
shipit_actors_other: []
automerge: automerge tests passed

click here for bot help

[felixfontein]

rebuild_merge

[felixfontein]

rebuild_merge

[felixfontein]

rebuild_merge

[felixfontein]

ok, I'm giving up.... ansibot doesn't seem to like me today...

[felixfontein]

I think the problem lies here:
https://github.com/ansible/ansibullbot/blob/master/ansibullbot/wrappers/defaultwrapper.py#L1277-L1285
This PR doesn't fit in one of the categories "exactly one commiter" (since some commits came from the GH Suggestion feature) and "number of commits exactly equals the number of commiters". That's why the third option is chosen: do nothing. (This code together with ansibot adding the automerge label above shows that it actually called merge() here.)

[felixfontein]

rebuild_merge

[felixfontein]

(CI run triggered manually via bugging people on IRC)

[felixfontein]

rebuild_merge

(yet another try...)

[felixfontein]

@hannseman thanks for the PR! Great work again (as usual ;) )!
@jwitko thanks for reviewing!
@gundalow thanks for merging!