docs: Document how to use GH workflows in forks #821
Conversation
And disable useless workflows runs in CI
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update the documentation and the CI/CD workflow configuration. A new section titled "Run tests in your fork" is added to the CONTRIBUTING file, providing visual guidance for enabling workflows in forked repositories, while redundant examples in the "Run and debug hooks locally" section are removed for clarity. In the CI/CD workflow file, conditional statements are introduced in the build and check jobs to refine event handling based on the repository context, specifically preventing push events for the upstream repository and allowing skips for certain jobs. Changes
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/CONTRIBUTING.md (1)
173-180: Address minor grammatical issues in the "Run tests in your fork" section.
In the newly added section:
- On line 175, the instruction reads: "Go to you fork
Actionstab and click on big green button". Consider changing "you fork" to "your fork" and prepend "the" to "big green button" for clarity.- On line 179, the phrasing "Now you can see are the tests pass before submitting PR." could be improved to read: "Now you can verify that the tests pass before submitting your PR."
Proposed diff:
-Go to you fork `Actions` tab and click on big green button +Go to your fork `Actions` tab and click on the big green button -Now you can see are the tests pass before submitting PR. +Now you can verify that the tests pass before submitting your PR.🧰 Tools
🪛 LanguageTool
[uncategorized] ~175-~175: “you” seems less likely than “your” (belonging to you).
Context: ... ``` ## Run tests in your fork Go to you forkActionstab and click on big gre...(AI_HYDRA_LEO_CP_YOU_YOUR)
[uncategorized] ~175-~175: You might be missing the article “the” here.
Context: ... to you forkActionstab and click on big green button 
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/contributing/enable_actions_in_fork.pngis excluded by!**/*.png
📒 Files selected for processing (2)
.github/CONTRIBUTING.md(2 hunks).github/workflows/ci-cd.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-cd.yml
89-89: property "branch" is not defined in object type {action: string; action_path: string; action_ref: string; action_repository: string; action_status: string; actor: string; actor_id: string; api_url: string; base_ref: string; env: string; event: object; event_name: string; event_path: string; graphql_url: string; head_ref: string; job: string; job_workflow_sha: string; path: string; ref: string; ref_name: string; ref_protected: string; ref_type: string; repository: string; repository_id: string; repository_owner: string; repository_owner_id: string; repositoryurl: string; retention_days: number; run_attempt: string; run_id: string; run_number: string; secret_source: string; server_url: string; sha: string; token: string; triggering_actor: string; workflow: string; workflow_ref: string; workflow_sha: string; workspace: string}
(expression)
🪛 LanguageTool
.github/CONTRIBUTING.md
[uncategorized] ~175-~175: “you” seems less likely than “your” (belonging to you).
Context: ... ``` ## Run tests in your fork Go to you fork Actions tab and click on big gre...
(AI_HYDRA_LEO_CP_YOU_YOUR)
[uncategorized] ~175-~175: You might be missing the article “the” here.
Context: ... to you fork Actions tab and click on big green button 
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: pre-commit
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
🔇 Additional comments (1)
.github/CONTRIBUTING.md (1)
16-16: New "Run tests in your fork" section added.
The new section provides clear guidance for enabling workflows in forked repositories. Please double-check that the link target (#run-tests-in-your-fork) matches the section header defined later in the document.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/CONTRIBUTING.md (1)
187-193: Enhance grammatical clarity in the "Run tests in your fork" section.
There are a couple of wording issues that could be improved for clarity and readability:
- Line 189: The phrase "Go to you fork
Actionstab and click on big green button" should be updated to "Go to your fork’sActionstab and click the big green button."- Line 193: The sentence "Now you can see are the tests pass before submitting PR." is unclear. Consider rephrasing to "Now you can verify that the tests pass before submitting your PR."
🧰 Tools
🪛 LanguageTool
[uncategorized] ~189-~189: “you” seems less likely than “your” (belonging to you).
Context: ... ``` ## Run tests in your fork Go to you forkActionstab and click on big gre...(AI_HYDRA_LEO_CP_YOU_YOUR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/CONTRIBUTING.md(2 hunks).github/workflows/ci-cd.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-cd.yml
🧰 Additional context used
🪛 LanguageTool
.github/CONTRIBUTING.md
[uncategorized] ~189-~189: “you” seems less likely than “your” (belonging to you).
Context: ... ``` ## Run tests in your fork Go to you fork Actions tab and click on big gre...
(AI_HYDRA_LEO_CP_YOU_YOUR)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: codecov/project/lib
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧹 Linters / pre-commit@🐍3.13@ubuntu-latest
- GitHub Check: 🧪 Tests / pytest@🐍3.10@macos-14
🔇 Additional comments (1)
.github/CONTRIBUTING.md (1)
16-16: Approve new table of contents entry.
The new navigation item "* Run tests in your fork" is well added and correctly links to the corresponding section heading.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci-cd.yml (1)
507-514: Refactor Suggestion: Use Immutable Repository ID in the 'check' JobIn the check job, the condition currently is:
if: >- always() && ( github.repository != 'antonbabenko/pre-commit-terraform' || github.event_name != 'push' || github.ref == 'refs/heads/master' )For improved robustness and consistency with the pre-setup job, consider using the immutable repository ID (as you already defined
UPSTREAM_REPOSITORY_ID) instead of comparing against a mutable repository name. This would help avoid issues if the repository name ever changes.Suggested Diff:
- github.repository != 'antonbabenko/pre-commit-terraform' + github.repository_id != 69382485This aligns the condition with the immutable identifier used elsewhere (and as recommended in previous reviews).
Double-check that this change still correctly guards the job for push events and is consistent with your overall workflow logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-cd.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-cd.yml
89-89: context "env" is not allowed here. available contexts are "github", "inputs", "needs", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
.github/workflows/ci-cd.yml
Outdated
| # GitHub branch protection about which check is required when multiple | ||
| # events trigger this workflow. | ||
| name: >- | ||
| ${{ github.event_name == 'pull_request' && 'check' || 'check ' }} |
There was a problem hiding this comment.
Btw, it works really nice
Screencast.from.24.02.25.21.41.10.webm
| if: >- | ||
| github.repository_id != needs.pre-setup.outputs.upstream-repository-id | ||
| || github.event_name != 'push' | ||
| || github.ref_name == github.event.repository.default_branch |
There was a problem hiding this comment.
I don't like this, tbh
Does it make sense to move that if from here (build) to tests and lint just to see 1 less skipped job with very odd name?
There was a problem hiding this comment.
There's nothing you can do about it. Tell GH to fix it.
There was a problem hiding this comment.
There was a problem hiding this comment.
I seem to have lost the track of this weird magic you guys are doing here, hence @MaxymVlasov please mention me when you think I should blind approve this PR =)
Should I approve it already? Asking since you requested review but still has some concerns expressed in comments after requesting approval 😕
ps: btw I'm a bit concerned about new commits not dismissing previous approve which might be used to push and merge changes that weren't reviewed and approved 🙈
That's already in branch ruleset (with some other changes), which waiting when @antonbabenko will return to laptop to apply it. That's part of #712 |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <webknjaz@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci-cd.yml (1)
532-543:⚠️ Potential issueResolve dependency context in the allowed-skips block.
Theallowed-skipsexpression referencesgithub.repository_id != needs.pre-setup.outputs.upstream-repository-idbut in the
checkjob’sneedssection (lines 519–521) onlylintandtestsare declared. As a result,needs.pre-setupis undefined in this context, which may lead to runtime errors or unexpected behavior according to static analysis (actionlint).Recommended Fix: Either add
pre-setupto theneedslist for thecheckjob or refactor the condition to use a variable available in the current context (for example, directly using the literal value defined in the env block). A possible diff to includepre-setupis:- needs: - - lint - - tests + needs: + - pre-setup + - lint + - testsThis change will ensure that
needs.pre-setup.outputs.upstream-repository-idis defined when evaluating the condition inallowed-skips.🧰 Tools
🪛 actionlint (1.7.4)
536-536: property "pre-setup" is not defined in object type {lint: {outputs: {}; result: string}; tests: {outputs: {}; result: string}}
(expression)
🧹 Nitpick comments (2)
.github/workflows/ci-cd.yml (2)
257-263: Review the build job’s conditional logic.
The conditional on these lines is designed to prevent runningpushevents for non-default branches of the upstream repository. The expression:if: >- github.repository_id != needs.pre-setup.outputs.upstream-repository-id || github.event_name != 'push' || github.ref_name == github.event.repository.default_branchensures the job runs only when either the repository ID is different, the event isn’t a push, or the branch is the default branch. While the logic is functionally sound, its complexity can impact readability. Consider refactoring or adding inline comments to clarify each condition.
511-515: Dynamic check job naming for clarity.
The change to dynamically alter the job name using:name: >- ${{ github.event_name == 'push' && 'check​' || 'check' }}is a clever trick to distinguish push events from others in the GitHub checks UI. Please verify that the use of a Zero Width Space (encoded as
​) is acceptable for your team’s naming conventions and does not conflict with branch protection rules.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-cd.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-cd.yml
536-536: property "pre-setup" is not defined in object type {lint: {outputs: {}; result: string}; tests: {outputs: {}; result: string}}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: 🧪 Tests / pytest@🐍3.9@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.13@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.9@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.13@macos-13
- GitHub Check: 🧪 Tests / pytest@🐍3.12@macos-14
- GitHub Check: 🧪 Tests / pytest@🐍3.11@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@windows-2025
- GitHub Check: 🧪 Tests / pytest@🐍3.10@ubuntu-24.04
🔇 Additional comments (1)
.github/workflows/ci-cd.yml (1)
55-55: Clarify the UPSTREAM_REPOSITORY_ID definition.
The environment variableUPSTREAM_REPOSITORY_IDis now explicitly set to 69382485 with an appropriate comment. This literal value is acceptable given that the upstream repository is fixed; however, ensure that its usage is consistent across all jobs and that any future changes to the upstream repo are properly updated here.
@yermulnik just check https://github.com/antonbabenko/pre-commit-terraform/pull/821/files with fresh eye disregard all previous discussions. All changes should be already documented, if not - please point to it |
|
This PR is included in version 1.97.4 🎉 |
Description of your changes
Didn't find a way to prevent pop-up 5 canceled jobs, but it is still better than tens of duplicated runs