[docker_container] Failing on non-string env values

[DBendit]

SUMMARY

Fixes #49802

Fail the module when non-string env values are used. Avoids issues (documented by @felixfontein below) around data loss due to YAML parsing.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

docker_container

ADDITIONAL INFORMATION

N/A

[ansibot]

cc @akshay196 @chouseknecht @cove @danihodovic @dariko @dusdanig @felixfontein @joshuaconner @jwitko @kassiansun @softzilla @tbouvet @zfil
click here for bot help

[felixfontein]

I'm not sure this is a good idea. We have no clue what exactly the user wanted (yes/true/True/..., no/false/False/...), and we currently simply select a random variant (true/false). It's probably better to fail and advise the user to quote this correctly. The official values accepted by YAML as booleans are y|Y|yes|Yes|YES|n|N|no|No|NO|true|True|TRUE|false|False|FALSE|on|On|ON|off|Off|OFF (see here).

The clean_dict_booleans_for_docker_api function has so far been used in situations where certain boolean strings are more likely (namely true and false). But here, any of these variants is equally likely.

Instead of making a random choice, we should fail when we find a non-string (the same problem is true if the user writes 1.00, which will end up as 1.0 by being converted to a float first -- depending on the context this is ok, or absolutely not ok).

[DBendit]

@felixfontein Alright, I've redone it all to error out on non-string values. I've had my fair share of issues with YAML in the past, but I didn't realize that data loss was one of them. Totally with you - failure (with a helpful error message) is better than silent data loss.

ready_for_review

[DBendit]

Updated.

ready_for_review

[felixfontein]

LGTM

[felixfontein]

bot_status

[ansibot]

Components

changelogs/fragments/49843-docker_container-wrap-env.yaml
support: community
maintainers:

lib/ansible/modules/cloud/docker/docker_container.py
support: community
maintainers: DBendit akshay196 chouseknecht cove danihodovic dariko dusdanig felixfontein joshuaconner jwitko kassiansun softzilla tbouvet zfil

test/integration/targets/docker_container/tasks/tests/options.yml
support: community
maintainers: DBendit akshay196 chouseknecht cove danihodovic dariko dusdanig felixfontein joshuaconner jwitko kassiansun softzilla tbouvet zfil

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 DBendit
shipit_actors_other: []
automerge: automerge !module file(s) test failed

click here for bot help

[felixfontein]

@DBendit thanks for the PR! Do you want to create a backport PR for it?
@sivel thanks for merging!

[felixfontein]

@DBendit please don't forget the backport PR for this and #49839 :-)

[DBendit]

@felixfontein I completely had! Thank you for the reminder. Still trying to get back into the swing of things post-holidays

[felixfontein]

@DBendit no problem :) There will probably be a new 2.7 release next week, so backports should probably be there until next Monday or so. (If you don't have time, I can also create them, just tell me.)

[felixfontein]

@DBendit I created a backport in #50899 for this one; I also noticed that the other one was a Feature PR, so it won't get backported anyway. I hope you don't mind me creating the backport, I just wanted to have it in 2.7.6 :)