docker_swarm_service: Extend env and add env_files support

[hannseman]

SUMMARY

This PR adds the possibility the specify env as a dict.

env:
  KEY1: VALUE1
  KEY2: VALUE2

It also adds a new option called env_files which accepts a list of paths to env-files.

env_files:
  - path/to/file.env
  - some/other/file.env

The order in this list matters where the last file will override variables already defined in previous files. It is useful for sharing env-files for different environments where a common file with defaults can be overridden by an environment specific file.

I chose to not map the behaviour of env_files to the docker-compose equivalent called env_file which accepts both a list and a string. Then we could not rely on the argument spec path type validation but would be required to implement our own validation that the specified files exist.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

docker_swarm_service

[ansibot]

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

[ansibot]

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

test/units/modules/cloud/docker/test_docker_swarm_service.py:70:31: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

[felixfontein]

TwoThree quick comments:

• type='path' in argument_spec does not check that the file exists, it only treats the argument like a path and does some basic tilde and variable expansion;
• type='list' also accepts strings and splits them by comma, and accepts ints and floats, so you need to emulate that behavior, too (see here for details; it might make sense to actually call that method of AnsibleModule so you don't have to reimplement all details);
• I think we should deprecate the non-dict form for env. Any opinions on this, in particular also from @dariko and @jwitko?

[hannseman]

type='path' in argument_spec does not check that the file exists, it only treats the argument like a path and does some basic tilde and variable expansion;

Ah yes, I somehow mixed that up.

Unfortunately the AnsibleModule-instance is not available in DockerService where validation / modification of arguments normally are handled. Do you think we should pass it to the DockerService class or create a new instance with an empty argument_spec (if that is possible?) or copy the details to our implementation?

[felixfontein]

You can always access it via client.module, if client is the AnsibleDockerClient instance.

[hannseman]

The DockerService is initialised like this:

new_service = DockerService.from_ansible_params(
    module.params,
    current_service,
    image_digest
)

Since only params is passed to DockerService.from_ansible_params, neither module or client is available. If module where to be passed instead of module.params it would then be required to be passed to the function get_docker_environment.

I guess that don't really need the int / float handling made in _check_type_list since that would generate an invalid environment anyway. If you agree then we could just add value.split(",") to get_docker_environment if the value is a string and then handle it as a list.

Edit: I made a commit which should make it a bit clearer.

[felixfontein]

self.env will never be None, it will always be a dict

[hannseman]

Hmm can it not be None from ansible parameters?

If no environment is parsed into env_list in get_docker_environment it will default to None:
https://github.com/hannseman/ansible/blob/68a2543f0bd0d2d44ef16b2575c43760e52aebd5/lib/ansible/modules/cloud/docker/docker_swarm_service.py#L600

[felixfontein]

Ah, sorry, I apparently completely missed the or None. Yes, you are totally right!

[felixfontein]

LGTM

[jwitko]

shipit

[felixfontein]

shipit

[felixfontein]

bot_status

[ansibot]

Components

changelogs/fragments/51762-docker_swarm_service-extend-env-and-add-env-file.yml
support: community
maintainers:

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/files/env-file-1
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

test/integration/targets/docker_swarm_service/files/env-file-2
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/units/modules/cloud/docker/test_docker_swarm_service.py
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): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 1
shipit_actors (maintainers or core team members): felixfontein jwitko
shipit_actors_other: []
automerge: automerge !module file(s) test failed

click here for bot help

[gundalow]

@hannseman Thank you for the PR
@felixfontein @jwitko Thank you for the reviews

Merged into devel for release in Ansible 2.8

[felixfontein]

Thanks everyone! :)