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

awx.awx.host enabled should not be passed by default #14461

Closed
5 of 11 tasks
sean-m-sullivan opened this issue Sep 19, 2023 · 4 comments
Closed
5 of 11 tasks

awx.awx.host enabled should not be passed by default #14461

sean-m-sullivan opened this issue Sep 19, 2023 · 4 comments
Labels
community component:awx_collection issues related to the collection for controlling AWX type:bug

Comments

@sean-m-sullivan
Copy link
Contributor

sean-m-sullivan commented Sep 19, 2023

Please confirm the following

  • I agree to follow this project's code of conduct.
  • I have checked the current issues for duplicates.
  • I understand that AWX is open source software provided for free and that I might not receive a timely response.
  • I am NOT reporting a (potential) security vulnerability. (These should be emailed to [email protected] instead.)

Bug Summary

If I don't pass the enabled option in the module, it should not be set.

Right now default python bool is false, so as is, the module behaves differently then the API.

https://github.com/ansible/awx/blob/f7a2de8a07dacc6384c8ce81b934d442aa14370a/awx_collection/plugins/modules/host.py#L115C8-L115C28

AWX version

devel

Select the relevant components

  • UI
  • UI (tech preview)
  • API
  • Docs
  • Collection
  • CLI
  • Other

Installation method

kubernetes

Modifications

no

Ansible version

N/A

Operating system

N/A

Web browser

No response

Steps to reproduce

Use host module.

- name: Add host
  host:
    name: localhost
    description: "Local Host Group"
    inventory: "Local Inventory"
    state: present
    controller_config_file: "~/tower_cli.cfg"
    variables:
      example_var: 123

Expected results

If I don't pass the enabled option in the module, it should not be set.

Right now default python bool is false, so as is, the module behaves differently then the API.

Actual results

Host is added but it is not enabled.

Additional information

No response

@github-actions github-actions bot added component:awx_collection issues related to the collection for controlling AWX needs_triage type:bug community labels Sep 19, 2023
@AlanCoding
Copy link
Member

I had to squint at the "Actual results" to realize that this was being reported elsewhere. The exact wording really gave me trouble:

Right now default python bool is false, so as is, the module behaves differently then the API.

You link the awx.awx.host module code, and a line containing the enabled python variable in it. This is not a "python bool". Nor does it have a default value. Because it has no default value, its python value in the problematic situation is None. I think what you mean to say is that the code/API converts None to false.

That behavior violates existing agreed-upon contracts in the collection, because None is well-established to denote the "not-provided" situation. We have discussed with @relrod (IIRC) that this is not ideal, because a proper python stand-in for not-provided would be better, but this results in hacking the collection infrastructure which frankly gives us more stuff to maintain that won't be sufficiently maintained.

I gave this thought, and 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.

My proposal would not work if a nullable field existed with a non-null default. In that case, the user would not be able to override a non-null default with a null value on creation. However, I don't think this exists. Nor do I think we are at risk of adding such a field. I don't want the best solution to block the good-enough solution, so I think we should run with the above proposal.

@relrod
Copy link
Member

relrod commented Sep 29, 2023

We have discussed with @relrod (IIRC) that this is not ideal, because a proper python stand-in for not-provided would be better, but this results in hacking the collection infrastructure which frankly gives us more stuff to maintain that won't be sufficiently maintained.

It's just not really possible. I've tried. I have tried various hacks. I have asked people on Core. There's just really no way, unless Core some day provides a sentinel value that can be used here, that isn't None. And I don't foresee that happening any time soon, because it will break a lot of existing stuff.

It makes me sad, but it is just a quirk that we kinda have to live with.

@AlanCoding
Copy link
Member

Sounds good, I put up a patch to resolve this #14493

@AlanCoding
Copy link
Member

Fix merged.

For the future - I believe this particular situation should usually only apply to boolean values... but there could be problems with ForeignKey fields as well. I believe that will wait for the present/enforced state options to be fully resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community component:awx_collection issues related to the collection for controlling AWX type:bug
Projects
None yet
Development

No branches or pull requests

4 participants