-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
collection bugfix booleans default value to match api #14528
Conversation
I was not able to get a k8's up to make good tests for the instances, if you can suggest, or I can address it here tomorrow. |
bd37045
to
5f85203
Compare
@@ -43,6 +43,7 @@ | |||
description: | |||
- If the host should be enabled. | |||
type: bool | |||
default: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to set the enabled
flag to true for any existing host where the option isn't specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right..........................................................................................................................................................................................................................................................................................................................................................................................................................................................
I think we both have felt the same frustration with the sea-saw, whack-a-mole that comes of this. Partially just from the anger I feel from multiple experiences of this, I don't want to leave the problem un-fixed, and I don't want to let structural code issues block a fix for such a simple problem. Your point here is well-taken. Today's fix should be more targeted, and avoid wading into the insanity of types of null. I've incorporated the spirit of your approach into #14493 Instead of marking the field deafults (which would mean maintaining more schema), that uses the module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not use this overall approach
I agree, this is just another can of worms in the long line of whack a mole, I don't think there is a great answer. Was thinking maybe we get the argument spec from the AAP API like I was thinking for enforced, but it still has that issue of if the user doesn't pass the field, it would default it. Right now if anything is passed it honors it, and the API will skip that field, but I think the answer to this goes back to the enforced defaults idea. I am trying to think of how we can make this better, and what criteria we would want on behavior of specifically host/shcedule state on
And focus on that behaving as expected, and how best to handle it. |
SUMMARY
collection bugfix booleans default value to match api
fixing issue
#14461
Also referencing
#14493
Also removes the tests folder from the final distribution tarball, I've seen a few instances where including it messes with sanity tests and others when collections like this are used as dependencies.
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION