TiCDC: Update presubmit job to skip make check on documentation and Dockerfile changes
#3975
Conversation
The `pull-check` job should not be skipped if only Go files are changed. The existing `skip_if_only_changed` definition was too broad and included Go files. - Created a new `make_check_skip_if_only_changed` definition. - Updated the `pull-check` job to use the new definition. Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @tenfyzhong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the presubmit configuration for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR refines the skip_if_only_changed pattern used by the pull-check presubmit job by introducing a new pattern make_check_skip_if_only_changed. The new pattern excludes markdown and common image file extensions, Dockerfiles, and owner files, aiming to skip the make check step when only documentation or Dockerfiles are changed. The change is straightforward and improves the accuracy of skipping unnecessary jobs, which can save CI resources. The code change is minimal and clean.
Code Improvements
-
Regex consistency and correctness
- File:
prow-jobs/pingcap/ticdc/latest-presubmits.yaml, lines 6-7 - The original
skip_if_only_changedpattern includes.yamland.json, but the newmake_check_skip_if_only_changedpattern excludes these extensions. If this is intentional (i.e.,.yamland.jsonchanges should cause the job to run), this should be explicitly documented in a comment to avoid confusion. Otherwise, it may cause unexpected skipping of checks on.yamlor.jsonchanges which might affect configuration files. - Suggestion: Add a comment above the new pattern explaining why
.yamland.jsonare excluded, or reconsider including them if they are relevant to code correctness.
# Skips make check if only documentation, images, Dockerfiles, or owner files changed. # Note: YAML and JSON files are excluded here to ensure make check runs on config changes. make_check_skip_if_only_changed: &make_check_skip_if_only_changed "(\\.(md|png|jpeg|jpg|gif|svg|pdf)|Dockerfile|OWNERS|OWNERS_ALIASES)$"
- File:
-
Regex pattern formatting
- The regex pattern is quoted but uses double backslashes
\\.which is correct for YAML string escaping, but it’s a bit hard to read. Consider using single quotes to reduce escaping or add a comment explaining the regex, improving maintainability.
- The regex pattern is quoted but uses double backslashes
Best Practices
-
Documentation
- File:
prow-jobs/pingcap/ticdc/latest-presubmits.yaml, lines 5-7 - Currently, there is no explanation in the YAML about the purpose of
make_check_skip_if_only_changedor its difference from the existingskip_if_only_changed. Adding a short comment describing the intent and scope of each pattern would help future maintainers.
- File:
-
Testing coverage
- The PR does not mention any tests or validation of the new skip pattern. It would be beneficial to add or verify existing tests that ensure the presubmit job is correctly skipped or triggered based on different file changes, especially for
Dockerfileand documentation files.
- The PR does not mention any tests or validation of the new skip pattern. It would be beneficial to add or verify existing tests that ensure the presubmit job is correctly skipped or triggered based on different file changes, especially for
Summary of Suggestions
# Define skip patterns for presubmit jobs
# `skip_if_only_changed` skips jobs if only docs, images, configs, or owner files changed
skip_if_only_changed: &skip_if_only_changed "(\\.(md|png|jpeg|jpg|gif|svg|pdf|yaml|json)|Dockerfile|OWNERS|OWNERS_ALIASES)$"
# `make_check_skip_if_only_changed` is more restrictive, skipping make check on docs, images, Dockerfiles, and owner files only.
# YAML and JSON files are excluded here to ensure make check runs on config changes.
make_check_skip_if_only_changed: &make_check_skip_if_only_changed "(\\.(md|png|jpeg|jpg|gif|svg|pdf)|Dockerfile|OWNERS|OWNERS_ALIASES)$"- Add or verify CI tests that validate the skipping behavior based on file changes.
- Clarify the intent of each pattern with comments for maintainability.
No critical issues identified; the change is minimal and clear. Adding documentation and test validation will make this improvement more robust.
tenfyzhong
changed the title
make check on documentation and Dockerfile changesmake check on documentation and Dockerfile changes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates the pull-check job to run when configuration files like .yaml or .json are changed, which improves CI coverage. The change is implemented by introducing a new, more specific skip_if_only_changed pattern. My review includes a suggestion to apply this logic consistently across other relevant Prow job configuration files to ensure uniform behavior.
| - ^release-8\.5$ | ||
| - ^feature/.+ | ||
| skip_if_only_changed: &skip_if_only_changed "(\\.(md|png|jpeg|jpg|gif|svg|pdf|yaml|json)|Dockerfile|OWNERS|OWNERS_ALIASES)$" | ||
| make_check_skip_if_only_changed: &make_check_skip_if_only_changed "(\\.(md|png|jpeg|jpg|gif|svg|pdf)|Dockerfile|OWNERS|OWNERS_ALIASES)$" |
There was a problem hiding this comment.
For consistency, this change should also be applied to other presubmit configurations. The file prow-jobs/pingcap/ticdc/release-9.0-beta-presubmits.yaml also contains a pull-check job that uses the broader skip pattern. Consider adding this new anchor to that file and updating its pull-check job to use it, ensuring consistent CI behavior across branches.
| - name: pull-check | ||
| decorate: true # need add this. | ||
| skip_if_only_changed: *skip_if_only_changed | ||
| skip_if_only_changed: *make_check_skip_if_only_changed |
There was a problem hiding this comment.
if there is only one place use this pattern, put the value directly here rather than use yaml anchor.
There was a problem hiding this comment.
We can merge this PR after the skip_if_only_changed rule works for other pipelines.
2 participants
This pull request updates the
skip_if_only_changedpattern for thepull-checkjob.The updated pattern
make_check_skip_if_only_changedspecifically excludes common non-code file extensions and Dockerfiles, ensuring that thepull-checkjob will run when relevant code changes are made.Changes include:
make_check_skip_if_only_changedthat is more precise.pull-checkjob.