node-core-utils
node-core-utils copied to clipboard
feat: ignore .md files when do `requiresJenkinsRun` check
This pr ignores .md files when do requiresJenkinsRun check
Refs: https://github.com/nodejs/node/pull/43483#issuecomment-1160210710
cc @aduh95
Codecov Report
Merging #641 (045233a) into main (460b50d) will increase coverage by
0.00%. The diff coverage is100.00%.
@@ Coverage Diff @@
## main #641 +/- ##
=======================================
Coverage 84.09% 84.09%
=======================================
Files 37 37
Lines 4074 4081 +7
=======================================
+ Hits 3426 3432 +6
- Misses 648 649 +1
| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/pr_checker.js | 98.29% <100.00%> (-0.16%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 460b50d...045233a. Read the comment docs.
covert to draft because I still need to add more tests for this feature to keep coverage increasing.
2. Even node itself does not use
.mdfor documentation only. For example,test/fixturescontains.mdfiles that affect tests (e.g., because they are linter test cases).
@tniessen those tests are covered by the GHA CI, I think it's safe to skip Jenkins CI for those.
- There is no guarantee that dependencies will use
.mdfor documentation only.
Sure, but that seems even more rare than what this PR is trying to fix. For those case, it's still possible to add the needs-ci label manually.
- Even node itself does not use
.mdfor documentation only. For example,test/fixturescontains.mdfiles that affect tests (e.g., because they are linter test cases).@tniessen those tests are covered by the GHA CI, I think it's safe to skip Jenkins CI for those.
I don't think it's a problem with the current state of the repository, but there is no guarantee that this won't change in the future.
- There is no guarantee that dependencies will use
.mdfor documentation only.Sure, but that seems even more rare than what this PR is trying to fix. For those case, it's still possible to add the
needs-cilabel manually.
Agreed, both false positives without this PR and false negatives with this PR are unlikely.
Anyway, I am not explicitly blocking this PR; erroring on the side of caution is merely my personal preference.
How often does this help, i.e., how often do PRs edit
*.mdfiles outside ofdoc?If it does not happen a lot, I am concerned about being too permissive.
- There is no guarantee that dependencies will use
.mdfor documentation only.- Even node itself does not use
.mdfor documentation only. For example,test/fixturescontains.mdfiles that affect tests (e.g., because they are linter test cases).
@tniessen Node core will add needs-ci label to all pr that modify files in test dir. If a pr modifies files in test/fixtures contains .md files that affect tests, the pr will have a needs-ci label and still require a Jenkins run.
This patch is useful in the below scenario:
- A pr makes a
.mdonly change and this change doesn't affect tests or other functional behavior. Even though, because the rule of pr label, this pr is added aneeds-cilabel and requires a Jenkins CI run to land in the commit queue. - A collaborator found this pr is just a doc change and remove the
needs-cilabel and then add the pr to the commit queue. - As a surprise, commit queue fail because node-core-utils still think a Jenkins CI run is required. This is a surprise.
This patch's motivation is to remove this surprise: If you do think this .md change only pr is not needed ci run, then remove needs-ci will lead to a commit queue success but not commit queue to fail.