Limit PR labeling

[Lennonka]

What changes are you introducing?

• Limiting PRs that will be labeled as Needs tech review and Needs style review to changes under the guides/ folder
• Limiting PRs that will be labeled as Needs testing to changes in procedure modules

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

• Changes outside the guides/ folder don't really need formal tech/style reviews, just community agreement
• Testing is typically only needed for procedures

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Contributor checklists

• I am okay with my commits getting squashed when you merge this PR.
• I am familiar with the contributing guidelines.

Please cherry-pick my commits into: Not sure how far we label

• Foreman 3.18/Katello 4.20 (Satellite 6.19)
• Foreman 3.17/Katello 4.19
• Foreman 3.16/Katello 4.18 (Satellite 6.18; orcharhino 7.6 and 7.7)
• Foreman 3.15/Katello 4.17
• Foreman 3.14/Katello 4.16 (Satellite 6.17; orcharhino 7.4; orcharhino 7.5)
• Foreman 3.13/Katello 4.15 (EL9 only)
• Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only; orcharhino 7.3)
• We do not accept PRs for Foreman older than 3.12.

[github-actions]

The PR preview for b80e0d1 is available at theforeman-foreman-documentation-preview-pr-4795.surge.sh

The following output files are affected by this PR:

• Configuring_virt_who_VM_Subscriptions/index-foreman-el.html

show diff

show diff as HTML

[Lennonka]

I don't understand why a guide is affected by this change... the diff should be clean.

[maximiliankolb]

LGTM; more input welcome.

[ekohl]

I think this misses some things. An assembly can be changed to include another pre-existing procedure (that isn't modified), resulting in some final change that does need to be tested. I'd be safe and accept some false positives over false negatives.

[Lennonka]

Thanks, Ewoud. In that case, I'll add assemblies as well.

[Lennonka]

Suggested change

	- any-glob-to-any-file: ['guides/common/modules/proc_*.adoc']
	- any-glob-to-any-file: ['guides/common/assembly_*.adoc','guides/common/modules/proc_*.adoc']

[ekohl]

I think you missed my point. I was suggesting to not change the line because the master.adoc files could also make a difference. Or attributes. If anything, it's probably guides/**/*.adoc.

[Lennonka]

@aneta-petrova What is your opinion on this?

[Lennonka]

@ekohl I disagree in principle because I don't think we should make functional changes in such a large scope. When we change master.adoc, it's most often a restructuring without functional changes. With attributes, I'm not sure, that's less than 50 % where attributes would be changed that affects procedures. And when the attribute change does affect procedures, it goes with changes in steps -> proc_* files. I'd go with adding the label manually for edge cases.

[Lennonka]

We can always try it out and change it later ;)

[aneta-petrova]

@aneta-petrova What is your opinion on this?

I haven't had a chance to take a closer look so I don't have one :) There are four clever people trying to get this right already, I don't think there's a need to throw my brain into the mix too ;)

[ekohl]

@ekohl I disagree in principle because I don't think we should make functional changes in such a large scope. When we change master.adoc, it's most often a restructuring without functional changes. With attributes, I'm not sure, that's less than 50 % where attributes would be changed that affects procedures. And when the attribute change does affect procedures, it goes with changes in steps -> proc_* files. I'd go with adding the label manually for edge cases.

And I'm still worried about missing those changes and think it's more important to avoid false negatives at the cost of some false positives.

There's no reason it can't be dropped later when there are too many false positives, but I think the changes to master.adoc and attributes are rare enough that still deserve a special look.

[Lennonka]

"Special look" is what the tech review is for, which is labeled for every change under guides/**. Testing means going through a procedure step-by-step.

[ekohl]

I don't understand why a guide is affected by this change... the diff should be clean.

Perhaps a race condition between merges?

[jafiala]

@Lennonka This is just a change to which labels are assigned automatically, correct? We can still manually add "needs testing" to whichever PR we need, right?

[Lennonka]

@jafiala Correct.

[ekohl]

This is just a change to which labels are assigned automatically, correct?

It only affects creation:

foreman-documentation/.github/workflows/labeler.yml

Lines 8 to 11 in 9a80761

	on:
	pull_request_target:
	types:
	- opened

So if you open an empty PR then it won't attach any labels, even if you later add files.

[Lennonka]

I'm going to merge this so that we can try it out. If we're not happy with it later, we can always change it back. I'll keep those commits separate.

And we still can add the labels manually if needed.