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.

Sign up for GitHub

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

Jump to bottom

Fix boolean defaults. #1343

Merged
merged 1 commit into from Nov 23, 2020
Merged

Fix boolean defaults. #1343

merged 1 commit into from Nov 23, 2020

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Fallout of ansible/ansible#72699.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

various

@ansibullbot
Copy link
Collaborator

cc @Akasurde @Andersson007 @DBendit @Dorn- @nitaco @Nosmoht @WojciechowskiPiotr @agronholm @ahtik @akshay196 @albertomurillo @alikins @amit0701 @andytom @angstwad @barnabycourt @bcoca @bennojoy @bgurney-rh @brontitall @carchi8py @chouseknecht @claco @colin-nolan @danihodovic @dariko @dermute @devyanikota @emonty @erjohnso @fishman @fxfitz @glitchcrab @haad @hnanni @hulquest @ilicmilan @jasperla @jbscalia @jpdasma @jwitko @kahowell @kamsz @kassiansun @kostiantyn-nemchenko @lmprice @lonico @machacekondra @matburt @mator @ndswartz @nerzhul @nibalizer @ovcharenko @pyykkis @rambleraptor @ravibhure @rosmo @russoz @sac @scathatheworm @schmots1 @scottanderson42 @sebasmannem @sgargan @smashwilson @srvg @tastychutney @tbouvet @tcraxs @timuster @troy2914 @xen0l
click here for bot help

@felixfontein
Copy link
Collaborator Author

@ndswartz @nerzhul @nibalizer @ovcharenko @pyykkis @rambleraptor @ravibhure @rosmo @russoz @sac @scathatheworm @schmots1 @scottanderson42 @sebasmannem @sgargan @smashwilson @srvg @tastychutney @tbouvet @tcraxs @timuster @troy2914 @xen0l

@felixfontein
Copy link
Collaborator Author

This needs to be merged first before any of the removal PRs can be merged, since we also need the fixes for the removed stuff in the stable-1 branch (where it's still there).

felixfontein
felixfontein commented Nov 22, 2020
View reviewed changes
Copy link
Collaborator Author

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three places are potentially problemated; I explained here why they are ok. This should make it easier to verify that this PR is ok.

plugins/modules/web_infrastructure/django_manage.py
@@ -270,7 +270,7 @@ def main():

apps=dict(default=None, required=False),
cache_table=dict(default=None, required=False, type='str'),
clear=dict(default=None, required=False, type='bool'),
clear=dict(default=False, required=False, type='bool'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All options in specific_boolean_params are only tested for being truthy, so None and False are teated the same way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think it is better to remove it, but other than that, using a default that is actually in the domain of values for the specified type does look better.

plugins/modules/storage/glusterfs/gluster_peer.py
@@ -141,7 +141,7 @@ def call_peer_commands(self):
def main():
module = AnsibleModule(
argument_spec=dict(
force=dict(type='bool', required=False),
force=dict(type='bool', required=False, default=False),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force is only tested for truthy in

community.general/plugins/modules/storage/glusterfs/gluster_peer.py

Line 91 in f03ca81

self.force = 'force' if self.module.params.get('force') else ''
, so False and None are treated the same way.

plugins/modules/cloud/rackspace/rax_clb.py
@@ -276,7 +276,7 @@ def main():
timeout=dict(type='int', default=30),
type=dict(choices=['PUBLIC', 'SERVICENET'], default='PUBLIC'),
vip_id=dict(),
wait=dict(type='bool'),
wait=dict(type='bool', default=False),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait is passed on into cloud_load_balancer, and is there only tested for being truthy:

Therefore False and None are treated the same way.

lonico
lonico approved these changes Nov 22, 2020
View reviewed changes
Copy link

@lonico lonico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved for na_cdot file

In general, we use a value of None to detect whether the value was set by the user.

@resmo
Copy link
Member

resmo commented Nov 22, 2020

verified influx_db params

Andersson007
Andersson007 approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgresql_set - should work, thanks @felixfontein

gundalow
gundalow approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this

@gundalow gundalow merged commit a96f90f into ansible-collections:main Nov 23, 2020
patchback bot pushed a commit that referenced this pull request Nov 23, 2020
@felixfontein
Fix boolean defaults. (#1343)
b6f57bb 
(cherry picked from commit a96f90f)
@patchback patchback bot mentioned this pull request Nov 23, 2020
Merged
@felixfontein felixfontein deleted the argument-spec branch November 23, 2020 11:17
@felixfontein
Copy link
Collaborator Author

@lonico @resmo @Andersson007 @gundalow thanks a lot for reviewing and merging!

@gundalow gundalow mentioned this pull request Nov 23, 2020
Merged
felixfontein added a commit that referenced this pull request Nov 23, 2020
@patchback @felixfontein
Fix boolean defaults. (#1343) (#1358)
247da98 
(cherry picked from commit a96f90f)

Co-authored-by: Felix Fontein <felix@fontein.de>
@Andersson007 Andersson007 mentioned this pull request Nov 23, 2020
Merged
@felixfontein felixfontein mentioned this pull request Apr 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russoz russoz russoz left review comments

@resmo resmo resmo approved these changes

@gundalow gundalow gundalow approved these changes

@lonico lonico lonico approved these changes

@Andersson007 Andersson007 Andersson007 approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug cloud clustering community_review database docker docs_fragments docs_fragments plugin (shared docs) gce identity module module net_tools os packaging plugins plugin (any type) postgresql storage system web_infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@felixfontein @ansibullbot @resmo @russoz @gundalow @lonico @Andersson007