Fix Boolean values defaulting to False in collection

[AlanCoding]

SUMMARY

Connect #14461

Execution of the proposal

I propose to remove all None values from the data in the method create_if_needed. By modifying the behavior of that method, it will not affect updates, and thus not prevent someone from changing a value from non-null to null. That may be valid in rare cases.

I can confirm the new assertion here fails in devel without this patch.

ISSUE TYPE

• Bug, Docs Fix or other nominal change

COMPONENT NAME

• Collection

[sean-m-sullivan]

I want to test this, but my only concern is that if the user passes "" as a value, does it get popped, and will it interfere when we start doing things with the enforced state in the future.

[AlanCoding]

if the user passes "" as a value, does it get popped

It should not, this is the reason for the is None condition.

But I should probably give a quick test to confirm that Ansible allows passing empty strings and carries it through the various contracts.

[AlanCoding]

Confirmed.

Passing in empty string for description:

  - name: Create a Host with exists
    host:
      name: foobar
      description: ""
      inventory: "Demo Inventory"

And then inspecting module.params in the awx.awx.host module confirms that we get

'description': '', 'inventory': 'Demo Inventory', 'state': 'present', 'controller_host': None,

So this shows the distinction between empty string inputs and inputs which are not provided.

[sean-m-sullivan]

It Seems good, and you are right for description it works. However for the Boolean that this originated on, #14461, it has the following behavior with your above task (name,descrip,inventory)

Host does not exist
Run 1, Host created, is enabled.
Run 2, Task Changed, Host is disabled
Run 3, Task ok, Host stays disabled

If you turn it on in the GUI, and then run again, it again disables the host.
I suspect its because python Bool is default false, and we may have to set all the Bools to default to what their API default is, which would likely be another PR, but still not behaving as epxected as is.
Code.
https://github.com/AlanCoding/awx/blob/7bb2ab3a46a3c2848ab87adc22c405cc9827ff33/awx_collection/plugins/modules/host.py#L84C1-L84C1

Did another test, where input was null, but where we set the default to '' in python, and it did pass and was not affected by this, which is what I was originally concerned about. So you are right on the enforced state, but found a 2nd bug.

[AlanCoding]

I'm going to be honest that I'm not understanding your steps. Could you share tasks that accomplish this reproducer?

Sure, we should consider whether True / False values are distinguished from None.

---
- name: Issue replication
  hosts: localhost
  gather_facts: false
  collections:
    - awx.awx

  tasks:
  - name: Create a Host with exists
    host:
      name: foobar
      description: ""
      inventory: "Demo Inventory"
      enabled: false

Running this, I find that module.params contains:

'enabled': False

So if you created a new host with that task, and then viewed it in the UI, it will be disabled. That's correct, the user intent in this case is to have a disabled host.

[AlanCoding]

Oh I see

---
- name: Issue replication
  hosts: localhost
  gather_facts: false
  collections:
    - awx.awx

  tasks:
  - name: Create a Host with exists
    host:
      name: foobar
      description: ""
      inventory: "Demo Inventory"

  - name: Should change nothing
    host:
      name: foobar
      description: ""
      inventory: "Demo Inventory"

The second task still disables the host.

[AlanCoding]

Ok, I see. This goes deep, and we seem to formally have a choice between different wrong behaviors.

[sean-m-sullivan]

yep, Sorry didn't see the notifications earlier today, dealing with different fun quirky behaviors. I'm going to see about a PR to fix the BOOL issue, I think the enabled is one of the few trues we have, but going to double check. I think this PR is good, just doesn't solve the BOOL issue :)

[AlanCoding]

@sean-m-sullivan after the last commits I pushed, I really do think it solves the "BOOL issue" you reference. This fixes it defaulting to False on creation, and being over-opinionated on edits. Any time an existing object exists, it will not change the enabled field unless that option is passed.

[sean-m-sullivan]

Tested, working as expected, Awesome!

[sean-m-sullivan]

This failed on

    "msg": "The conditional check 'lookup('awx.awx.controller_api', 'hosts/15/').enabled' failed. The error was: The lookup `awx.awx.controller_api` was found, however lookups were disabled from templating"

In my previous PR I came accross this and switched to this to fix it

- name: Use lookup to check that host was enabled
  ansible.builtin.set_fact:
    host_enabled_test: "lookup('awx.awx.controller_api', 'hosts/{{result.id}}/').enabled"

- name: Newly created host should have API default value for enabled
  assert:
    that:
      - host_enabled_test

[sean-m-sullivan]

Failure is here, I tried to find out where this changed in ansible, but newer ansible does not like lookups in asserts.

[AlanCoding]

yeah you're right, I'll use the test from your patch