node-core-utils icon indicating copy to clipboard operation
node-core-utils copied to clipboard

feat: ignore .md files when do `requiresJenkinsRun` check

Open F3n67u opened this issue 3 years ago • 5 comments

This pr ignores .md files when do requiresJenkinsRun check

Refs: https://github.com/nodejs/node/pull/43483#issuecomment-1160210710

cc @aduh95

F3n67u avatar Jul 12 '22 14:07 F3n67u

Codecov Report

Merging #641 (045233a) into main (460b50d) will increase coverage by 0.00%. The diff coverage is 100.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 data Powered by Codecov. Last update 460b50d...045233a. Read the comment docs.

codecov[bot] avatar Jul 12 '22 14:07 codecov[bot]

covert to draft because I still need to add more tests for this feature to keep coverage increasing.

F3n67u avatar Jul 12 '22 15:07 F3n67u

2. Even node itself does not use .md for documentation only. For example, test/fixtures contains .md files 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.

  1. There is no guarantee that dependencies will use .md for 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.

aduh95 avatar Jul 22 '22 15:07 aduh95

  1. Even node itself does not use .md for documentation only. For example, test/fixtures contains .md files 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.

  1. There is no guarantee that dependencies will use .md for 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.

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.

tniessen avatar Jul 22 '22 16:07 tniessen

How often does this help, i.e., how often do PRs edit *.md files outside of doc?

If it does not happen a lot, I am concerned about being too permissive.

  1. There is no guarantee that dependencies will use .md for documentation only.
  2. Even node itself does not use .md for documentation only. For example, test/fixtures contains .md files 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:

  1. A pr makes a .md only change and this change doesn't affect tests or other functional behavior. Even though, because the rule of pr label, this pr is added a needs-ci label and requires a Jenkins CI run to land in the commit queue.
  2. A collaborator found this pr is just a doc change and remove the needs-ci label and then add the pr to the commit queue.
  3. 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.

F3n67u avatar Jul 23 '22 10:07 F3n67u