search

ansible-community / community-team

For issues to be done by the Community Team that don't fall into a particular repository
0 stars 2 forks source link

Open issues on collections that use booleans to recommend true/false #60

Closed samccann closed 1 year ago

samccann commented 2 years ago

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.

samccann commented 2 years ago

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:

samccann commented 2 years ago

See https://github.com/ansible-collections/amazon.aws/issues/1041 for an example on how to open a collection issue and what to include.

samccann commented 2 years ago

What I did: 1 - go to the collection on https://docs.ansible.com/ansible/devel/collections/

  2. check off that collection in https://github.com/ansible-community/community-team/issues/60#issuecomment-1252688300
tremble commented 2 years ago

community.aws is done already - https://github.com/ansible-collections/community.aws/pull/1420 (no need to open issue)

felixfontein commented 2 years ago

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).

Andersson007 commented 2 years ago

fyi: I've created an issue in community.mysql and I'm keeping it open advertising as an easyfix for newcomers

kristianheljas commented 1 year ago

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?

samccann commented 1 year ago

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?

kristianheljas commented 1 year ago

So .. double and triple checked, I'm ready to launch the issue strike against 96 collections, based on unchecked collections under https://github.com/ansible-community/community-team/issues/60#issuecomment-1252688300 (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!

kristianheljas commented 1 year ago

Issue for openstack.cloud done: https://storyboard.openstack.org/#!/story/2010586

samccann commented 1 year ago

@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!

kristianheljas commented 1 year ago

@samccann, no need to beg - happy to contribute! :)

1) Added auto-generation disclosure and call to freely close this issue, see this HackMD Enhance it if you wish :)

2) 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.

Andersson007 commented 1 year ago

@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?

kristianheljas commented 1 year ago

@Andersson007 I don't think I can link to docs.ansible.com, since the core team is not on board with this decision, see https://github.com/ansible/ansible/pull/77581#issuecomment-1105656254, 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 https://github.com/ansible/ansible-lint/pull/1954#pullrequestreview-900985805.

@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.

Andersson007 commented 1 year ago

@kristianheljas @samccann i believe news-for-maintainers is to announce stuff relevant for collection maintainers, not for core maintainers.

tremble commented 1 year ago

Given that there is now a Community decision https://github.com/ansible-community/community-topics/issues/116 and antsibull-docs now converts over to true/false https://github.com/ansible-community/antsibull-docs/pull/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?)

felixfontein commented 1 year ago

@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...

samccann commented 1 year ago

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.

kristianheljas commented 1 year ago

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)

samccann commented 1 year ago

@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 astrue/falsefor consistency and compatibility withansible-lint.

samccann commented 1 year ago

@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... :-)

kristianheljas commented 1 year ago

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:

  1. Create the news-for-maintainers issue with the same content as the issue (except the last two sentences)
  2. Fire the 96 issues away
kristianheljas commented 1 year ago

@samccann, issues have been launched!

Out of 96 collections to notify, there were 2 exceptions:

  1. community.azure - It's archived now and content has been yanked, so thats fine
  2. 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

kristianheljas commented 1 year ago

@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/false for boolean values in documentation and examples, we ask that you evaluate booleans in this collection > and consider changing any that do not use true/false (lowercase).

See documentation block format for > more info (specifically, option defaults).

mariolenz commented 1 year ago

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?

samccann commented 1 year ago

@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 commented 1 year ago

@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"?

kristianheljas commented 1 year ago

@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!

mariolenz commented 1 year ago

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

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 ;-)

felixfontein commented 1 year ago

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'
felixfontein commented 1 year ago

(Please note that this will flag most GitHub workflows that contain on:. Simply ignore these matches :) )

tremble commented 1 year ago

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.

mariolenz commented 1 year ago

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!

tremble commented 1 year ago

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
mariolenz commented 1 year ago

(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:

https://github.com/ansible-collections/community.vmware/blob/6c133cb92f7669d7decc3ab866fb700e866159be/plugins/modules/vmware_host_service_manager.py#L203

I'll ignore this, just FYI.

samccann commented 1 year ago

@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.

felixfontein commented 1 year ago

(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:

https://github.com/ansible-collections/community.vmware/blob/6c133cb92f7669d7decc3ab866fb700e866159be/plugins/modules/vmware_host_service_manager.py#L203

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.

mariolenz commented 1 year ago

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.

Thanks! I didn't know this, I'll fix it.

edit: vmware_host_service_manager: Fix example

mariolenz commented 1 year ago

@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.

samccann commented 1 year ago

@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.

mariolenz commented 1 year ago

@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.

samccann commented 1 year ago

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!