Classify/Label PRs That Are Docs Only

[sommersoft]

This is an approach to address #1047.

Working with the currently available labels in ansible/ansible:

• Will add the docsite docs_only label to PRs if ALL changes are limited to "docs only" logic.
• Will remove a docsite docs_only label if there are changes outside of "docs only", unless the label was applied by a human.
• Will not override/remove any other labels that the triager determines are applicable.

I couldn't determine with any certainty how ansibullbot adds the docsite label (via utils/component_tools.py, etc.); it seemed mostly random looking at various PRs and the component matching routines.

This is written quite narrowly at the moment, but could possibly be expanded for use in other issues (notification, standards enforcement, etc).

Very small sample set (PRs are now merged):

##################################################################
# ansible/ansible PR: 73287                                      #
# Changes: 1. The `DOCUMENTATION` variable in postgresql_user.py #
##################################################################
$ ./triage_ansible.py --dry-run --debug --verbose --pr 73287 --logfile ../ansibot_log/log.txt --ignore_galaxy

2021-01-25 08:17:13,383 INFO 4670 ansible.py:__init__:228 starting bot
....
2021-01-25 08:17:51,694 INFO 4670 ansible.py:run:524 url: https://github.com/ansible/ansible/pull/73287
2021-01-25 08:17:51,694 INFO 4670 ansible.py:run:525 title: [2.9] postgresql_user: add clarification about no_password_changes option
2021-01-25 08:17:51,695 INFO 4670 ansible.py:run:528 component[f]: lib/ansible/modules/database/postgresql/postgresql_user.py
2021-01-25 08:17:51,695 INFO 4670 ansible.py:run:541 needs_revision
{'assign': [],
 'cancel_ci': False,
 'cancel_ci_branch': False,
 'close': False,
 'close_migrated': False,
 'comments': [],
 'merge': False,
 'newlabel': ['docsite', 'needs_ci', 'needs_revision'], # <--- docsite label added by get_docs_facts()
 'open': False,
 'rebuild': False,
 'rebuild_failed': False,
 'unassign': [],
 'uncomment': [],
 'unlabel': ['collection:community.postgresql', 'community_review']}
Dry-run specified, skipping execution of actions


##################################################################
# ansible/ansible PR: 73334                                      #
# Changes: 1. Added a changelog fragment                         #
#          2. The `DOCUMENTATION` & `RETURNS` variables, and     #
#             several logic changes in apt_key.py                #
#          3. Changes to integration tests                       #
##################################################################
$ ./triage_ansible.py --dry-run --debug --verbose --pr 73334 --logfile ../ansibot_log/log.txt --ignore_galaxy

2021-01-25 08:22:26,017 INFO 5551 ansible.py:__init__:228 starting bot
....
2021-01-25 08:23:17,367 INFO 5551 ansible.py:run:524 url: https://github.com/ansible/ansible/pull/73334
2021-01-25 08:23:17,372 INFO 5551 ansible.py:run:525 title: change detection and check mode fixes for apt_key
2021-01-25 08:23:17,372 INFO 5551 ansible.py:run:528 component[f]: changelogs/fragments/apt_key_fixes.yml
2021-01-25 08:23:17,372 INFO 5551 ansible.py:run:528 component[f]: lib/ansible/modules/apt_key.py
2021-01-25 08:23:17,372 INFO 5551 ansible.py:run:541 needs_revision
{'assign': [],
 'cancel_ci': False,
 'cancel_ci_branch': False,
 'close': False,
 'close_migrated': False,
 'comments': [],
 'merge': False,
 'newlabel': ['needs_ci'], # <--- docsite label not added by get_docs_facts()
 'open': False,
 'rebuild': False,
 'rebuild_failed': False,
 'unassign': [],
 'uncomment': [],
 'unlabel': []}
Dry-run specified, skipping execution of actions

The attached logfile contains more examples (50 open PRs at time of running).
log_new_docsite.txt

Open questions:

1. Are the directory matching patterns the desired ones?
2. The AST parsing and comparison functions ignore module-level/non-class functions. Is this correct?
3. How are Galaxy components affected? I didn't see any activity from this addition when running my tests with Galaxy included, but I'm not certain there are no effects.
4. Is a test desired for this?
   • Unit test created for get_docs_facts.
5. Is docsite the appropriate label to assign, or should a new label be created on ansible/ansible?
   • docs_only label created, and will be used.

[lgtm-com]

This pull request introduces 1 alert when merging 7d9af8c into fe79383 - view on LGTM.com

new alerts:

• 1 for Unnecessary 'else' clause in loop

[codecov]

Codecov Report

Merging #1505 (3f1aaea) into master (3f6c438) will increase coverage by 0.73%.
The diff coverage is 91.09%.

@@            Coverage Diff             @@
##           master    #1505      +/-   ##
==========================================
+ Coverage   59.60%   60.33%   +0.73%     
==========================================
  Files          55       56       +1     
  Lines        8250     8447     +197     
==========================================
+ Hits         4917     5096     +179     
- Misses       3333     3351      +18     

[mkrizek]

Thanks for working on this.

I was a bit confused at first because the description mentions adding docsite label but in fact this should add a new label something like docs_only instead of re-using existing docsite label that is used for pull requests that change content of docs/docstite/ directory (documentation website).

I haven't looked at the code at detail yet, just quicky skimmed through get_docs_facts and left some comments there.

Are the directory matching patterns the desired ones?

Maybe use docs/ instead of just docs/docsite?

The AST parsing and comparison functions ignore module-level/non-class functions. Is this correct?

I am not sure. Why ignore those?

How are Galaxy components affected? I didn't see any activity from this addition when running my tests with Galaxy included, but I'm not certain there are no effects.

Anything that is in galaxy should be auto-closed anyway so we don't have to take them into account.

Is a test desired for this?

Yes, please.

cc'ing @acozine @samccann for any input from the docs team.

[sommersoft]

@mkrizek, thanks for the review. Your suggestions (along with LGTM's) did highlight that I should've polished a little more before submitting. Some of those are left-over after munging my standalone prototype into ansibullbot.

...the description mentions adding docsite label but in fact this should add a new label something like docs_only instead of re-using existing docsite...

I agree that docs_only, or something similar and separate, would be better. I just wanted to work with a label that existed in the ansible/ansible repo to get this started. I actually meant to add this to the Open Questions (will edit/expand after this message).

The AST parsing and comparison functions ignore module-level/non-class functions. Is this correct?

I am not sure. Why ignore those?

My thinking was that most of Ansible's documented API is class based, making module-level functions a rarity for the docs. However, Ansible is a large project, so this could entirely be false; especially for edge cases. It wouldn't take much to add them. Would just need to rearrange the data structure and add another conditional.

Is a test desired for this?

Yes, please.

After codecov ran, I figured that would be the case. 😄

[acozine]

Thanks @sommersoft for a detailed PR! This sent me back to look at the labels more generally.

We have three "docs-related" labels: docs, docsite, and docsite_pr (see descriptions). The docsite_pr label is meant for PRs that use the Edit on GitHub feature. I'm not sure that label is being applied correctly, but the intention is clear and, if correctly applied, useful. The other two labels are less clearly differentiated in my mind, and the descriptions don't help very much. It would be great to make the descriptions and the logic clear.

If I'm reading this PR correctly, it would use docsite for PRs and issues that ONLY affect the docs, and docs for PRs and issues that include both docs changes and other changes. That seems reasonable, but a broader discussion would be worthwhile, so we can update the descriptions as well as the code. Can we add this to the DaWGs agenda for next week?

[sommersoft]

If I'm reading this PR correctly, it would use docsite for PRs and issues that ONLY affect the docs, and docs for PRs and issues that include both docs changes and other changes.

As it is now, this PR will only apply docsite for PRs that only affect the docs. Issues are not evaluated, and docs is not applied in any case.

We have three "docs-related" labels: docs, docsite, and docsite_pr (see descriptions). The docsite_pr label is meant for PRs that use the Edit on GitHub feature. I'm not sure that label is being applied correctly, but the intention is clear and, if correctly applied, useful. The other two labels are less clearly differentiated in my mind, and the descriptions don't help very much. It would be great to make the descriptions and the logic clear.

As @mkrizek pointed out and I agree with, a new docs_only label might work better for what this PR is doing. However, if one of the current label descriptions is defined to capture this meaning, using one of them could work too. This is all about making things easier for the humans, after all.

Can we add this to the DaWGs agenda for next week?

Certainly.

[mkrizek]

@acozine Should we include lib/ansible/plugins/doc_fragments/ as well?

[mkrizek]

Also, if bot determines that a PR is docs_only and only other changes in addition to docs only are in changelogs/fragments, we might add the label anyway?

[mkrizek]

Thanks for adding the tests @sommersoft!

Overall it looks good. I added comments but most of them are just suggestions, there are a few that need to be addressed though.

[sommersoft]

I've updated to using get_files() and all associated logic. The test changes weren't as trivial as I expected, but aren't too major.

I apologize for adding to the review workload on this!

[lgtm-com]

This pull request introduces 1 alert when merging a17d72b into f725f6c - view on LGTM.com

new alerts:

• 1 for Unused import

[mkrizek]

Thanks!

[mkrizek]

@sommersoft I see there is WIP in title, was there anything else you wanted to add here?

[sommersoft]

I see there is WIP in title, was there anything else you wanted to add here?

I don't have anything else. Still awaiting an answer from acozine on your question of additional exclusions, but I don't think that should necessarily hold this up.