Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ignore .md files when do requiresJenkinsRun check #641

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

F3n67u
Copy link
Member

@F3n67u F3n67u commented Jul 12, 2022

This pr ignores .md files when do requiresJenkinsRun check

Refs: nodejs/node#43483 (comment)

cc @aduh95

@codecov
Copy link

codecov bot commented Jul 12, 2022

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%) ⬇️

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.

@F3n67u F3n67u marked this pull request as draft July 12, 2022 14:59
@F3n67u F3n67u marked this pull request as ready for review July 12, 2022 14:59
@F3n67u F3n67u marked this pull request as draft July 12, 2022 15:00
@F3n67u
Copy link
Member Author

F3n67u commented Jul 12, 2022

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

@F3n67u F3n67u marked this pull request as ready for review July 12, 2022 15:34
lib/pr_checker.js Outdated Show resolved Hide resolved
lib/pr_checker.js Outdated Show resolved Hide resolved
@F3n67u F3n67u force-pushed the ignore-md branch 2 times, most recently from 494fed4 to 662b459 Compare July 12, 2022 15:38
@F3n67u F3n67u requested a review from aduh95 July 12, 2022 15:40
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@aduh95
Copy link
Contributor

aduh95 commented Jul 22, 2022

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.

@tniessen
Copy link
Member

  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.

@F3n67u
Copy link
Member Author

F3n67u commented Jul 23, 2022

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.

@aduh95 aduh95 merged commit 62f266f into nodejs:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants