Don't convert nulls to strings.

[feanil]

This change is similar to #10465

It extends the logic there to also support none types. Right now if you have
a '!!null' in yaml, and that var gets passed around, it will get converted to
a string.

eg. defaults/main.yml

ENABLE_AWESOME_FEATURE: !!null # Yaml Null
OTHER_CONFIG:
  secret1: "so_secret"
  secret2: "even_more_secret"

CONFIG:
  hostname: "some_hostname"
  features:
    awesame_feature: "{{ ENABLE_AWESOME_FEATURE}}"
  secrets: "{{ OTHER_CONFIG }}"

If you output CONFIG to json or yaml, the feature flag would get represented in the output
as a string instead of as a null, but secrets would get represented as a dictionary. This is
a mis-match in behaviour where some "types" are retained and others are not. This change
should fix the issue.

I also updated the template test to test for this and made the changes to v2.

Added a changelog entry.

@abadger A type we missed when we made the change for numbers and booleans.

[feanil]

@abadger thoughts on this change?

[feanil]

@bcoca @abadger is there hesitation about this or just haven't gotten time to look at this yet?

[feanil]

@bcoca @abadger checking in on this PR. Any timeline on when you might have time to look at it?

[bcoca]

we might need to reformat this as devel has changed drastically.

[abadger]

@bcoca looks like it's been rebased since the switchover to v2 so it's probably okay in that regard. I've tested and it does work without further rebasing.

One thing I've noted is that implicit nulls are also changed by this PR:

---
- hosts: localhost
  vars:
    test_null: !!null
    test_empty:
    integer: 1
    string: "1"
    templated_dict:
      null: "{{ test_null }}"
      empty: "{{ test_empty }}"
      integer: "{{ integer }}"
      string: "{{ string }}"
  tasks:
    - template:
        src: null.j2
        dest: /var/tmp/null.txt

the variable "empty" is also changed from empty string to null.

Not sure if that behaviour change might break some things. @bcoca suggested that we make this configurable in the config file and set it to empty string by default for backwards compat.

[feanil]

@abadger this matches the behaviour of how python generally treats an empty value in yaml. I would think that we would want to match that. If it seems like a dangerous change, I'm happy to put it behind a config flag. Can you point me to an example that I can work from?

feanil@edx ~/src/test$ cat empty.yml 
foo:
bar: ""
feanil@edx ~/src/test$ python
Python 2.7.9 (default, Feb 23 2015, 17:11:37) 
[GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import yaml
>>> yaml.safe_load(open('empty.yml'))
{'foo': None, 'bar': ''}
>>> 

[abadger]

@feanil, yeah... the problem being backwards compatibility :-( Since empty values have implicitly mapped to an empty string for such a long time there's probably quite a few playbooks that will break from that behaviour changing. Making it a configurable value would hopefully let us change the default from backwards compatible to more in line with what other things do in the future.

@bcoca do you happpen to know of a good simple config setting that feanil can use as an example of writing one for this feature?

[bcoca]

fail_on_undefined had very similar scope and fix

[amenonsen]

@feanil, are you planning to resubmit this with a config-variable guard? (Maybe something like null_representation defaulting to an empty string?) The change seems worthwhile.

[feanil]

@amenonsen yes, I'm hoping to get some time in the next week or so to work on this.

[feanil]

@bcoca @amenonsen Added a commit to make this configurable per your suggestions, please let me know if there is anything more I should do.

[abadger]

@feanil cool. So the ChangeLog and the code don't agree here. the ChangeLog says that the default has changed to None but the code defaults to returning an empty string. The core team talked about this for a while this morning and we decided that since there's a need to specify a None in playbooks that consistently propogates out to module parameters that we'd be okay with switching the default for v2 and then documenting it prominently.

So three changes and then we can merge:

• Add the new config variable for None to the ChangeLog. Mention that the none change means that empty variables now equate to None instead of empty string.
• Change the default in constants.py
• Add a test for the empty var case in the integration tests.

[feanil]

@bcoca @abadger I've addressed all the requested changes. There is a test currently that tests the new default behavior for yaml nulls. However I didn't see any way to also test with a different config so right now there is no test that tests that the override is working as expected.

[feanil]

@bcoca @abadger rebased to be mergable again.

[feanil]

@bcoca @abadger checking in on this again in case there is anything else that needs to be done?

[feanil]

Rebased to be mergable again.

[abadger]

Thanks @feanil ! Merged.

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

• https://groups.google.com/forum/#!forum/ansible-project - for user questions, tips, and tricks
• https://groups.google.com/forum/#!forum/ansible-devel - for strategy, future planning, and questions about writing code

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!