Uh oh!
There was an error while loading. Please reload this page.
feat: ignore .md files when do requiresJenkinsRun check - #641
Conversation
Codecov Report
@@ Coverage Diff @@## main #641 +/- ##
=======================================
Coverage 84.09% 84.09% =======================================
Files 37 37 Lines 4074 4081 +7 =======================================
+ Hits 3426 3432 +6 - Misses 648 649 +1
Continue to review full report at Codecov.
|
covert to draft because I still need to add more tests for this feature to keep coverage increasing. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
494fed4 to
662b459Compare
tniessen
left a comment
There was a problem hiding this comment.
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.
- 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 those tests are covered by the GHA CI, I think it's safe to skip Jenkins CI for those.
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 |
tniessen
commented
Jul 22, 2022
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.
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. |
F3n67u
commented
Jul 23, 2022
@tniessen Node core will add This patch is useful in the below scenario:
This patch's motivation is to remove this surprise: If you do think this |
This pr ignores .md files when do
requiresJenkinsRuncheckRefs: nodejs/node#43483 (comment)
cc @aduh95