ansible / ansible

Ansible is a radically simple IT automation platform that makes your applications and systems easier to deploy and maintain. Automate everything from code deployment to network configuration to cloud management, in a language that approaches plain English, using SSH, with no agents to install on remote systems. https://docs.ansible.com.
https://www.ansible.com/
GNU General Public License v3.0
62.49k stars 23.83k forks source link

DRAFT: Add YAML tags for controlling variable merge behavior #83934

Closed phemmer closed 3 hours ago

phemmer commented 3 weeks ago
SUMMARY

This adds a few YAML tags for controlling how variables are merged when there are duplicate names across multiple sources.

There are 2 commits in the PR. The first commit adds a !merge tag, and is for controlling merge behavior of maps/hashes/dictionaries/whatever_you_want_to_call_them. The second commit adds !append and !prepend for controlling merging of lists/arrays. This is a bit more complex of a change, as Ansible sometimes will merge the same variables multiple times, resulting in a list being (pre/ap)pended multiple times. Maps don't have this issue as it'll just result in a key overwrite with the same value. So for lists we have to track where the list came from to prevent duplicate entries.
Note that the merge behaviors are not automatically recursive. The tag must be applied at each level to merge that level (otherwise it replaces).

This is definitely a draft, as there are things I would probably clean up and re-arrange. However with the code as it is, it makes it clearer what's going on. There also would need to be tests, addressing the linter, etc. Right now it's just up so it can be reviewed conceptually before proceeding.

I've tested this with group_vars, host_vars, playbook vars, block vars, task vars, & role vars.

Closes #83903

While DEFAULT_HASH_BEHAVIOUR is also deprecated-but-not-quite, and has a messy history (#72021, #72669, #73089, #73328, #74215), this change will hopefully provide an actionable replacement for the functionality, and allow it to truly be deprecated.

ISSUE TYPE
ADDITIONAL INFORMATION
# group_vars/all
foo:
  one: one
  two:
    two_dot_one: two dot one
  three:
    - three dot one
    - three dot two
# host_vars/localhost
foo: !merge
  one: hostvars one
  three: !prepend
    - three dot zero
# playbook.yml
- hosts: localhost
  roles:
    - test
  vars:
    foo: !merge
      four: four
# roles/test/defaults/main.yml
foo: !merge
  five: five

^Note, this is not shown in the output below because it's a default, and thus overridden. Just showing to demo the behavior that defaults are not merged if override is present.

# roles/test/tasks/main.yml
- block:
  - debug: var=foo
    vars:
      foo: !merge
        six: six
      blockvar: blockvar
  vars:
    foo: !merge
      seven: seven
    taskvar: taskvar
TASK [test : debug] ************************************************************
task path: /tmp/at/roles/test/tasks/main.yml:2
ok: [localhost] => {
    "foo": {
        "four": "four",
        "one": "hostvars one",
        "seven": "seven",
        "six": "six",
        "three": [
            "three dot zero",
            "three dot one",
            "three dot two"
        ],
        "two": {
            "two_dot_one": "two dot one"
        }
    }
}
Review

A few questions that need answering:

ansibot commented 2 weeks ago

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

lib/ansible/parsing/yaml/constructor.py:185:1: E302: expected 2 blank lines, found 1
lib/ansible/parsing/yaml/constructor.py:205:1: E305: expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/parsing/yaml/constructor.py:190:16: disallowed-name: Disallowed name "_"
lib/ansible/parsing/yaml/constructor.py:197:16: disallowed-name: Disallowed name "_"

click here for bot help

bcoca commented 2 weeks ago

https://yaml.org/type/merge.html

phemmer commented 2 weeks ago

https://yaml.org/type/merge.html

I'm not sure what you're getting at. I don't see any relation to that, and this change. They're completely different. This change is about merging variables of the same name across sources (multiple yaml documents consumed independently by python code). That functionality you linked is about merging multiple nodes of different names within the same document.

nitzmahone commented 1 week ago

YAML tags aren't really the best mechanism for this- the existing Ansible usages are arguably an abuse of what was supposed to be a type hint for deserializers and schema validators. They're not usable as a parallel data plane (ala XML attributes)- you only get one per node, so if you need another one (eg, to override a problematic YAML implicit resolver like bool or octal int, or to signal more than one custom behavior), you're hosed. They also cause a lot of load/interop problems with other tooling. We're stuck with !vault and !unsafe for the foreseeable future, but we'll likely never add any more.

The oft-deferred data tagging mechanism (now slated for 2.19) supports a fully composable tagging scheme that serves as that additional metadata plane, and we've already successfully prototyped a per-var dynamic replacement for hash_behaviour: merge that allows for variable-level opt-in merge behavior like this (in a way that doesn't break the world for portable content like existing HB:M does).

We definitely haven't forgotten- I was probably the loudest voice inside the core team for "we need to keep HB:M until we've got a suitable replacement" (much as I detest its current impl), and we're on the cusp of being able to do it "correctly"...