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 support for a flexible container_args to avoid implementing every docker run command line argument #60

Open
4 tasks
andygrunwald opened this issue Mar 11, 2023 · 5 comments
Milestone

Comments

@andygrunwald
Copy link
Contributor

andygrunwald commented Mar 11, 2023

Context

This Ansible module uses its own parameters to support various docker run command line arguments.
Out of those, multiple types are used:

  • Single strings
  • Lists

Examples for single strings:

  • container_network maps to --network
  • container_user maps to --user
  • container_hostname maps to --hostname

Examples for lists:

  • container_volumes maps to --volume
  • container_ports maps to --publish
  • container_hosts maps to --add-host

This is great and marks explicitly how to use this module.
The big downside on this: You need to add additional support for each docker run command line argument into this module. And the docker run reference is looooong.

One proposal would be to make this Ansible more flexible and avoid the burden of implementing every single docker run command line argument by offering ways to add arbitrarily

  • key -> value pairs (for single string values that are only allowed once in a docker run cmd, like --user)
  • key -> list[value] pairs (for docker run cmd arguments that are allowed multiple times like --volume)

How a possible Ansible usage can look like in the future / Usage of the module:

- name: Start image
  include_role:
    name: mhutter.docker-systemd-service
  vars:
    container_name: "hello-world"
    container_image: "hello:v1"
    container_docker_pull_force_source: false

    container_single_args:
      network: my-network
      user: nonroot
      hostname: world
      [...]

    container_multi_args:
      publish:
        - '127.0.0.1:3030:3000'
        - '127.0.0.1:1234:5678'
      volume:
        - '/opt/foo:/var/lib/bar:rw'
      [...]

    [...]

The top-level keys under container_single_args and container_multi_args are not validated They can be defined to whatever you want.

A few keys, like container_image might be good to keep separate, primarily when those are used in a different context (e.g., to craft the systems unit name).

The same can be done for boolean flags via a list like container_boolean_args for keys like --rm or --tty:

container_boolean_args:
  - 'rm'
  - 'tty'

Open decisions

  • Define which keys need to stay separate and used in different contexts
  • Align on a naming for single arguments, boolean arguments, multi list arguments
  • Align on how to do it (strict breaking change + removing the keys or "smooth" migration via deprecating
  • Implementation: Align on several small pull requests or one big one (to keep the default branch working)

Additional notes

  • This would indicate a breaking change to the module and a v1.x might be needed for a release
  • It might be good to deprecate or directly remove the previous parameters that are replaced by the new capability. Removing directly would free up the code base directly.
  • Changelog + Migration guide would be required
@andygrunwald
Copy link
Contributor Author

@mhutter, I would appreciate your thoughts on this.

Personally, I am just for a strict breaking change with changelog entry and migration guide + releasing a v1.X after this. Even with -alpha or -beta suffix at first.

@mhutter
Copy link
Owner

mhutter commented Mar 13, 2023

Hey, this is pretty much what I had in mind! Good catch with single string vs lists; I thought of just accepting both, but not all parameters can be specified once.

I would however argue that we should not try to validate this in this module.

It might even be possible to just have a container_args parameter, which accepts both key->value as well as key->list pairs (IIRC Jinja2 can do some magic with that). We don't even have to handle booleans, docker will also accept --rm=true etc.

A few keys, like container_image might be good to keep separate, primarily when those are used in a different context (e.g., to craft the systems unit name).

Yes I agree, for example mandatory things (name, image), or thing where the module offers some kind of abstraction (env, maybe ports and volumes - where to draw the line?).

@andygrunwald
Copy link
Contributor Author

or thing where the module offers some kind of abstraction (env, maybe ports and volumes - where to draw the line?).

What abstraction do you have in mind?

@mhutter
Copy link
Owner

mhutter commented Apr 28, 2023

I think for a v3 I'd set up the following:

Explicit variables for mandatory & positional parameters

Examples: container_name, container_image, container_args (or container_cmd as it is currently called)

Explicit variables for "abstract" options

Things like container_docker_pull that don't map to a command line option or argument.

container_opts for everything else

Just a map of strings & lists that can be expanded into individual options, examples:

container_opts:
  memory: 1G
  privileged: true
  health-cmd: curl ....

I also thought about having explicit variables for commonly used options (like ports, volumes, env vars) but after thinking about it, this brings almost no advantages while at the same while making both usage and implementation more complicated.

@andygrunwald WDYT?

@mhutter mhutter added this to the v3 milestone Apr 28, 2023
@ednxzu
Copy link

ednxzu commented Dec 9, 2023

Hello,

I've cloned your role to make it work better for my usecase, and I am currently working on this feature. Here is the repo in case you want to have a look. I simplifies the template quite a bit in the process, tho I haven't tested it heavily yet to make sure it can handle all sorts of flags being passed.

I can make a PR for it if you're interested.

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

No branches or pull requests

3 participants