-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(ci): only markdown action to skip some CI workflows #13118
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
base: master
Are you sure you want to change the base?
feat(ci): only markdown action to skip some CI workflows #13118
Conversation
@kamuik16 you've got an outdated filecoin-ffi here - I think you branched just at the wrong moment and now have the wrong ffi; rebase on to master and make the ffi change go away and 🤞 your tests should pass |
…/only-markdown-lint
e232ee3
to
3c50c54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a composite GitHub Action to detect when a PR includes only Markdown file changes and integrates a planner job in several workflows to skip heavy CI steps for docs-only PRs.
- Added a new composite action for detecting Markdown-only changes.
- Integrated a planner job into the stale, docker, check, and build workflows to conditionally skip jobs.
- Updated workflow dependencies to run jobs only when non-doc changes are present.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
.github/workflows/stale.yml | Added a planner job and conditional trigger in the stale workflow. |
.github/workflows/docker.yml | Added a planner job and corresponding if condition in the docker workflow. |
.github/workflows/check.yml | Added a planner job and updated job dependencies in the check workflow. |
.github/workflows/build.yml | Added a planner job and conditional step to skip build when docs-only. |
.github/actions/only-markdown/action.yml | Added a composite action to determine if only Markdown files changed. |
Comments suppressed due to low confidence (1)
.github/workflows/stale.yml:15
- [nitpick] Consider aligning the output variable name between the planner job (currently 'only_docs') and the composite action output ('only_markdown_changes') to improve clarity and consistency.
only_docs: ${{ steps.check.outputs.only_markdown_changes }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave to others to give approval. I wish we didn't need to make checks in every job of of a workflow, but per #13069 I didn't see any way to accomplish this, so your approach seems right.
- uses: actions/checkout@v4 | ||
with: | ||
submodules: 'recursive' | ||
fetch-depth: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed given .github/actions/only-markdown/action.yml
is doing the checkout as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not checked out, it shows this error: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/home/runner/work/lotus/lotus/.github/actions/only-markdown'. Did you forget to run actions/checkout before running your local action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the checkout code from the composite action, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. And to be clear, it doesn't work to just put the checkout code in the composite action along and remove it from the workflow? I'm assuming not.
.github/workflows/build.yml
Outdated
name: Detect docs-only changes | ||
runs-on: ubuntu-latest | ||
outputs: | ||
only_docs: ${{ steps.check.outputs.only_markdown_changes }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than only_docs
, I wonder if we call this continue
.
Would that just lets us say
if: ${{ needs.planner.outputs.continue }}
to keep it concise and to keep all the business logic inside the planner job of whether to continue (rather than leaking out this decision to whether the planner says there are only docs changes).
Not strongly held opinion, just a thought. this can always be adjusted further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is the best way to achieve this under the constraints.
For this, this PR itself can be an example (where non .md files changes and build/tests were ran), but I need to think some other way to test only for .md changes. |
If this isn't easy, I wouldn't worry about this. In the worst case, it's not working as desired for markdown only changes and we need to do followups. |
I have tested the commands locally, and it works perfectly fine, and as you said, worst case, the build and test runs, if that happens, which I'm pretty sure won't, I'll fix it. |
Related Issues
Closes #13069
Proposed Changes
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that: