Open issues on collections that use booleans to recommend true/false
samccann opened this issue · 42 comments
As part of the community decision to standardize on true/false for boolean values, We need to open issues on the collections that use these to recommend they follow this practice.
Using this issue to track which collections we open the issues on. NOTE we cannot mandate the collection owner use true/false so we will only track that we opened the issues with that recommendation.
List of collections (Ansible 7 based). Each one should be checked to see if they use booleans and if so, open an issue in that repository:
- amazon.aws
- ansible.netcommon
- ansible.posix
- ansible.utils
- ansible.windows
- arista.eos
- awx.awx
- azure.azcollection
- check_point.mgmt
- chocolatey.chocolatey
- cisco.aci
- cisco.asa
- cisco.dnac
- cisco.intersight
- cisco.ios
- cisco.iosxr
- cisco.ise
- cisco.meraki
- cisco.mso
- cisco.nso
- cisco.nxos
- cisco.ucs
- cloud.common
- cloudscale_ch.cloud
- community.aws
- community.azure
- community.ciscosmb
- community.crypto
- community.digitalocean
- community.dns
- community.docker
- community.fortios
- community.general
- community.google
- community.grafana
- community.hashi_vault
- community.hrobot
- community.libvirt
- community.mongodb
- community.mysql
- community.network
- community.okd
- community.postgresql
- community.proxysql
- community.rabbitmq
- community.routeros
- community.sap
- community.sap_libs
- community.skydive
- community.sops
- community.vmware
- community.windows
- community.zabbix
- containers.podman
- cyberark.conjur
- cyberark.pas
- dellemc.enterprise_sonic
- dellemc.openmanage
- dellemc.os10
- dellemc.os6
- dellemc.os9
- dellemc.powerflex
- dellemc.unity
- f5networks.f5_modules
- fortinet.fortimanager
- fortinet.fortios
- frr.frr
- gluster.gluster
- google.cloud
- grafana.grafana
- hetzner.hcloud
- hpe.nimble
- ibm.qradar
- ibm.spectrum_virtualize
- infinidat.infinibox
- infoblox.nios_modules
- inspur.ispim
- inspur.sm
- junipernetworks.junos
- kubernetes.core
- lowlydba.sqlserver
- mellanox.onyx
- netapp.aws
- netapp.azure
- netapp.cloudmanager
- netapp.elementsw
- netapp_eseries.santricity
- netapp.ontap
- netapp.storagegrid
- netapp.um_info
- netbox.netbox
- ngine_io.cloudstack
- ngine_io.exoscale
- ngine_io.vultr
- openstack.cloud
- openvswitch.openvswitch
- ovirt.ovirt
- purestorage.flasharray
- purestorage.flashblade
- purestorage.fusion
- sensu.sensu_go
- splunk.es
- theforeman.foreman
- t_systems_mms.icinga_director
- vmware.vmware_rest
- vultr.cloud
- vyos.vyos
- wti.remote
See ansible-collections/amazon.aws#1041 for an example on how to open a collection issue and what to include.
What I did:
1 - go to the collection on https://docs.ansible.com/ansible/devel/collections/
2. - look at some of the module pages and search for boolean. If you see it, click on the issue tracker or repository links on the main collection page and open an issue similar to ansible-collections/amazon.aws#1041
3. check off that collection in #60 (comment)
community.aws is done already - ansible-collections/community.aws#1420 (no need to open issue)
community.crypto, community.dns, community.docker, community.hrobot, community.interal_test_tools (not listed here), community.routeros, and community.sops are also done. community.general is partially done; I'm sure I missed some cases, but the bulk should be done (I hope).
fyi: I've created an issue in community.mysql and I'm keeping it open advertising as an easyfix for newcomers
checking against ansible-build-data/7/collection-meta.yaml i noticed 4 missing collections: dellemc.unity, dellemc.powerflex, lowlydba.sqlserver and grafana.grafana.
Should they be notified as well?
yeah for sure. Thanks for checking! I updated the checklist with those four.
@Andersson007 - is there a way we can add some items that new collections should do so we don't have a growing list of collections to... bug about things like this?
So .. double and triple checked, I'm ready to launch the issue strike against 96 collections, based on unchecked collections under #60 (comment) (except community.sops which should be checked) using this script
Only exception is openstack.cloud which is not hosted at github, which I'll create manually.
When I get a second yay for this count (96) and this exclusion list, I'll go nuclear!
Issue for openstack.cloud done: https://storyboard.openstack.org/#!/story/2010586
@kristianheljas that's great! Can I beg you for a couple of enhancements?
1 - can you mention in the BODY that this issue is being autogenerated and please close if you have already implemented this?
2 - in some future iteration - could we extend this script 'somehow' so we can add in different issue titles, body text, and exclusion list? We have at least two other issues we'd like collection owners to consider and it would be helpful to have a script to run similar to this one.
No biggie if that's not possible. I 'think' I understand the script well enuf to at least edit it and ask for feedback/help in going nuclear on those other collection wish-list items. This is great stuff!
@samccann, no need to beg - happy to contribute! :)
-
Added auto-generation disclosure and call to freely close this issue, see this HackMD
Enhance it if you wish :) -
For sure, I didn't put much thought into this as i figured this as a one-off script, but i think it can be easily expanded to cover the wish-list as well. If this is a recurring thing, I can even put more thought/work on it. Ping me on GH/Matrix whenever you feel like!
P.S. This "exlusion list" actually records all the issue links it creates, and populating it before running will make it ignore these collections.
@kristianheljas @samccann are we talking about notifying maintainers about the doc standard change?
If yes, I would also suggest putting a link to the doc standards to the issues, what do you think?
@Andersson007 - is there a way we can add some items that new collections should do so we don't have a growing list of collections to... bug about things like this?
Do you mean a) how to notify maintainers or b) adding this in the collection requirement?
- a) we have the news-for-maintainers repo to notify collection maintainers. @kristianheljas please create the issue there (if it's not already there) as well with links to related decision and doc standard on docs.ansible.com
- b) there's a statement in the collection requirements that new collection must satisfy doc standards (w/o details as it'd be too noisy - reviewers should know the standards)
- c) i would also recommend announcing it via Bullhorn
@Andersson007 I don't think I can link to docs.ansible.com, since the core team is not on board with this decision, see ansible/ansible#77581 (comment), so the docsite shows yes/no values.
Then again, ansible-lint is, and will continue to be, to conform to YAML 1.2 spec, see ansible/ansible-lint#1954 (review).
@samccann Do you happen to know if this standard is documented somewhere besides the voting topic? At a glance, I didn't find this in collection requirements.
@kristianheljas @samccann i believe news-for-maintainers is to announce stuff relevant for collection maintainers, not for core maintainers.
Given that there is now a Community decision ansible-community/community-topics#116 and antsibull-docs now converts over to true/false ansible-community/antsibull-docs#19 if it's not documented, we should update the documentation (somewhere)
Additionally, if possible add a linting rule that prefers the strings "true"/"false". My understanding is that we can update the collections linting ruleset without updating the core ruleset (@felixfontein would such a linting rule be possible? - are they still strings by the time the linter sees them?)
@tremble ansible-test uses PyYAML to load the YAML, which converts both yes/no and true/false to the corresponding Python booleans. So in validate-modules it's not possible to check for this.
What would be possible to adjust the yamllint config that is used to validate YAMLs in collections. But I'm pretty sure core won't want to allow this, since the yamllint config used is very, very lenient, and I don't think they want to use it to force any convention on users. But I guess we could ask them...
The closest we have for documentation is at https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_variables.html#boolean-variables
@Andersson007 I 'think' most of the requirements for a valid collection are still in https://github.com/ansible-collections/overview/blob/main/collection_requirements.rst not in docs.ansible.com yet.
As for core not being onboard, they would not support something that forces it (Felix's prior comment I think) but are no longer against updating docs (or docs within collections) to use true/false since that community topic vote occurred.
The best I found collection requirements is a link to Development conventions, which suggests yes/no/true/false (in that order). Not even gonna mention the boolean variables @samccann linked that goes even further, including y/1/1.0...
But .. that's really great that the core team is ready to accept this change in the docs!
I'm gonna review the docsite built with updated deps anyway, I think I can get this wheel going during that.
I'll likely have an update on this on Monday.
@samccann @Andersson007 Should we postpone this mass-notification until we have some docs/requirements to support this or send these based on the voting topic alone to get things going in parallel?
And are the mass-issues necessary if we announce it on news-for-maintainers?
(I only need to push the big red button to send them away, so no complaints from me either way)
@kristianheljas I see value in opening issues as a reminder. We have a number of enhancements like this that collection owners aren't acting on, so this seems like a way to remind them directly. But I'm open to other views on this one.
As for changing the docs, the module development guidelines should be updated. Currently it says:
If the option is a boolean value, you can use any of the boolean values recognized by Ansible: (such as true/false or yes/no). Choose the one that reads better in the context of the option.
It should say something like:
If the option is a boolean value, end users can use any of the boolean values recognized by Ansible: (such as true/false or yes/no). Document booleans as true/falsefor consistency and compatibility withansible-lint.
@Andersson007 @tremble @Andersson007 - we were going to do the batch issue creation as soon as the docs are updated to say folks should document booleans as true/false.
Scream now if you're against this batch issue creation against all the collections that haven't done this already... :-)
Created a PR for the guidelines here and updated the issue template. Feel free to make any applicable modifications to the template.
Once the PR has made it's way to docs.ansible.com and no-one has complained, I will:
- Create the news-for-maintainers issue with the same content as the issue (except the last two sentences)
- Fire the 96 issues away
@samccann, issues have been launched!
Out of 96 collections to notify, there were 2 exceptions:
- community.azure - It's archived now and content has been yanked, so thats fine
- f5_modules.f5networks - They don't have issues enabled, although they have GH issues link in thery galaxy manifest. I'll ping them in the next comment.
Links for 94 issues created can be found in this gist (and above this comment, hehe)
I included the previously manually created openstack.cloud for completeness as well
@wojtek0806, we wanted to open an issue to f5_modules.f5networks about collection standards change but it appears you do not have an active issue tracker (manifest points to GH and GH issues are disabled this repo).
Can you please have a look and update the manifest or open the GH issue tracker?
Also, here is the notification we wanted to send:
Based on the community decision to use >
true/falsefor boolean values in documentation and examples, we ask that you evaluate booleans in this collection > and consider changing any that do not usetrue/false(lowercase).See documentation block format for > more info (specifically, option defaults).
I fear that the answer will be "no", but anyway: Are there any scripts or tools to help maintainers with this, at least to find out what needs to be changed?
And another thing: We use ansible-network/collection_prep in community.vmware to create the docs. I might be wrong, but it looks to me that it uses yes / no when documenting booleans. At least, it didn't update the documentation to true / false when I ran it to create ansible-collections/community.vmware#1661. Am I correct, or do I overlook something?
@mariolenz can you open an issue on that collection_prep? I'm not familiar with it but sounds like it's not following along on the true/false change.
@mariolenz can you open an issue on that collection_prep? I'm not familiar with it but sounds like it's not following along on the true/false change.
@samccann ansible-network/collection_prep#81
If anyone can point me to a better way to create the docs, feel free to do it. I'm not above replacing collection_prep if there are better alternatives ;-)
OK, we've dealt with my second question. The answer to my first one is... "no"?
@mariolenz I don't think there is, the best i can think of right now is something like :\s(yes|no|True|False|y|n)\b in regex, targeting just docstrings.
If your IDE supports to search only inside strings (i know Intellij ones do) then you might be able make your life even easier.
Regardless, if you are able to only reflect this in the source docstrings, thats a big win as well as it bring consistency to the ansible docsite, which i think was the main driving force behind all this.
Thanks for opening the issue in collection_prep!
I don't think there is, the best i can think of right now is something like
:\s(yes|no|True|False|y|n)\bin regex, targeting just docstrings
This is my approach, too. I'll do my best, but if you don't have a tool to check this it's only "best effort" from my side. I hope to fix this, but as I've said: Only best effort ;-)
If you use ansible-test devel from a checkout, you can apply the following diff to check at least some things:
diff --git a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/default.yml b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/default.yml
index 45d8b7adcf..5ca29b7bbe 100644
--- a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/default.yml
+++ b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/default.yml
@@ -16,4 +16,8 @@ rules:
new-line-at-end-of-file: disable
new-lines: {type: unix}
trailing-spaces: disable
- truthy: disable
+ truthy:
+ level: error
+ allowed-values:
+ - 'true'
+ - 'false'
diff --git a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/modules.yml b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/modules.yml
index da7e604999..d1add23b51 100644
--- a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/modules.yml
+++ b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/modules.yml
@@ -16,4 +16,8 @@ rules:
new-line-at-end-of-file: disable
new-lines: {type: unix}
trailing-spaces: disable
- truthy: disable
+ truthy:
+ level: error
+ allowed-values:
+ - 'true'
+ - 'false'
diff --git a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/plugins.yml b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/plugins.yml
index 6d41813787..d1add23b51 100644
--- a/test/lib/ansible_test/_util/controller/sanity/yamllint/config/plugins.yml
+++ b/test/lib/ansible_test/_util/controller/sanity/yamllint/config/plugins.yml
@@ -11,9 +11,13 @@ rules:
empty-lines: disable
hyphens: disable
indentation: disable
- key-duplicates: disable
+ key-duplicates: enable
line-length: disable
new-line-at-end-of-file: disable
new-lines: {type: unix}
trailing-spaces: disable
- truthy: disable
+ truthy:
+ level: error
+ allowed-values:
+ - 'true'
+ - 'false'(Please note that this will flag most GitHub workflows that contain on:. Simply ignore these matches :) )
Looking at community.aws I've noticed that a couple of regressions have crept it. While we can whack-a-mole these, unless there's something we can add to CI, they will keep creeping back in.
If you use ansible-test devel from a checkout, you can apply the following diff to check at least some things
Thanks @felixfontein, this really helps!
If you use ansible-test devel from a checkout, you can apply the following diff to check at least some things:
I really wish there was a way for collections to override some of these default settings. Some of the disabled rules would actually be nice to enable
(Please note that this will flag most GitHub workflows that contain
on:. Simply ignore these matches :) )
You can also tweak the workflow YAML files to tell yaml-lint to ignore the lines (and still grumble at other truthy-ness issues) https://yamllint.readthedocs.io/en/stable/disable_with_comments.html
on: # yamllint disable-line rule:truthy
(Please note that this will flag most GitHub workflows that contain
on:. Simply ignore these matches :) )
This also flags something like service_policy: on for a parameter that isn't a boolean:
I'll ignore this, just FYI.
@mariolenz - what do you use the docs for when you use collection_prep? Is it just to test that the docs generate? OR are you putting them up on some website somewhere?
Galaxy beta (aka GalaxyNG) displays module/plugin docs now so there's less need for collection owners to generate module rst files and keep them in their repo.
(Please note that this will flag most GitHub workflows that contain
on:. Simply ignore these matches :) )This also flags something like
service_policy: onfor a parameter that isn't a boolean:
Well, but on and off aren't strings in YAML 1.1, but booleans. For example if you run yaml.safe_load('service_policy: on') with PyYAML, you'll end up with {'service_policy': True}.
So the warning is correct and you should always put on and off in quotes, because otherwise they get converted to booleans.
Well, but
onandoffaren't strings in YAML 1.1, but booleans. For example if you runyaml.safe_load('service_policy: on')with PyYAML, you'll end up with{'service_policy': True}.So the warning is correct and you should always put
onandoffin quotes, because otherwise they get converted to booleans.
Thanks! I didn't know this, I'll fix it.
@mariolenz - what do you use the docs for when you use collection_prep? Is it just to test that the docs generate? OR are you putting them up on some website somewhere?
Galaxy beta (aka GalaxyNG) displays module/plugin docs now so there's less need for collection owners to generate module rst files and keep them in their repo.
We generate the docs and put them into the docs directory since the VMware modules have been moved out of ansible and into a dedicated collection. We don't push them to any other website.
How do tother collections handle module documentation? If it's the default to not have this in the repo, I can change it.
@mariolenz - all those module docs are visible today at https://beta-galaxy.ansible.com/ui/repo/published/community/vmware/content/module/vcenter_domain_user_group_info/ for example. So once Galaxy NG is ..beyond beta, you can probably stop using the collection_prep and just point to your collection on GalaxyNG for the module docs.
@samccann I could also link to docs.ansible.com, I just never did. And since we're discussing moving to a new domain(ansible-community/community-topics#201) and Galaxy NG is still beta, I'd like to wait a bit before I decide what to do.
Anyway, I think I've fixed this in most places in the DOCUMENTATION and EXAMPLES blocks.
The purpose of this issue was to create the reminder issues in each collection, so going to close this out as done. Thanks @kristianheljas for the script help and everyone else for the help!