remove push directive so we will not run tests on merge#9144
Conversation
WalkthroughThe GitHub Actions workflow configurations were updated to remove the "push" event as a trigger from three workflows. Now, these workflows are only triggered by "pull_request" and, where applicable, "workflow_dispatch" events. Minor clarifying comments were also updated in one workflow file. Changes
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
nadavMiz
force-pushed
the
remove-push-directive
branch
3 times, most recently
from
77acde0 to
a74a6f8
Compare
Signed-off-by: nadav mizrahi <nadav.mizrahi16@gmail.com>
nadavMiz
force-pushed
the
remove-push-directive
branch
from
a74a6f8 to
849ea70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/Validate-package-lock.yaml (1)
2-2: Consider retainingworkflow_dispatchfor ad-hoc validationsThe other two workflows still allow manual runs (
workflow_dispatch). Keeping the same trigger here would let maintainers validatepackage-lock.jsonon demand without opening a PR..github/workflows/run-pr-tests.yaml (1)
69-70: Fix typo in comment for clarity- #on dispach, use the current branch + #on dispatch, use the current branch
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/Validate-package-lock.yaml(1 hunks).github/workflows/jest-unit-tests.yaml(1 hunks).github/workflows/run-pr-tests.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.933Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
.github/workflows/run-pr-tests.yaml (1)
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.933Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.
🔇 Additional comments (2)
.github/workflows/jest-unit-tests.yaml (1)
2-2: Confirm test coverage on protected branches after push removalDropping the
pushtrigger removes noisy duplicates, but it also means this workflow will not run for direct commits pushed to protected branches (e.g., urgent hot-fixes done without a PR). If your policy mandates all changes go through PRs, that’s fine; otherwise consider re-adding a branch-filteredpushormerge_grouptrigger to keep the safety net..github/workflows/run-pr-tests.yaml (1)
2-2: Push trigger removal: verify release workflow expectationsAs with the Jest workflow, ensure that relying solely on
pull_request/workflow_dispatchwon’t skip test execution for straight-to-main merges or release branch pushes that bypass PRs.
2 participants
Describe the Problem
We currently run GitHub Actions for pull request (PR) tests on both
pushandpull_requestevents. However, since we push changes to a forked branch, thepushevent triggers a workflow run on the forked branch, and again on the target branch (e.g.,noobaa) when the PR is merged—because the merge itself is treated as a push to the target branch.This results in redundant test executions. Additionally, the workflows triggered by the push event often fail, making these runs both unnecessary and noisy.
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit