説明
Overview
I think it's time to start iterating on https://github.com/hashicorp/consul/pull/11536 . Since, I've gathered some feedback from folks that we can implement.
Suggestions
Let's look at an errored check:
PR seems to modify metrics behavior. It seems no tests or test behavior has been modified.
Please update the PR with any relevant updated testing or add a pr/no-metrics-test label to skip this check.
S1: Which branch/ conditional was triggered?
Let's have the script indicate which branch failed. That is, was there a metric mentioned found in the "title of the pr", "body", or the actual diff?
For instance, a better UX would be:
{ PR diff seems to modify metrics behavior/ PR title mentions metrics/ PR body mentions metrics }
It seems no tests or test behavior has been modified.
Please update the PR with any relevant updated testing or add a pr/no-metrics-test label to skip this check.
S2: Check the actual diff changes
Let's look at a pseudo diff hunk:
// this line mentions the word "metric"
- deletion
- deletion
+ addition
+ addition
While the diff does not actually contain "metric" in the deletions or additions, the word is already in the code and appears in the diff. This will trigger the text checker and could fail the script. It'd be valuable if the script only checked lines starting with - / + in the diff.
S3: Run only on push/ commits to PRs
Today, the checker runs on any GitHub event, including adding/removing labels. It would be better to only run on commits.
S4: Check diff against PR base branch
Today, the checker diffs against origin/main ( ref ). This is not actually ideal.
For instance, if the base of the PR is not main, the checker is not that relevant. Further, if the branch is not rebased on the latest origin/main, the check may look at changes outside the PR.
Note: In this section,
originis a remote that points togit@github.com:hashicorp/consul.git.
Testing
Since the checker is actually a bash script, contributors can test most of their changes locally. This may require setting appropriate env vars that the bash script looks at. Another way to test would be to use https://github.com/nektos/act/ .