ci: remove manage runs workflow #2325
Conversation
WalkthroughThe pull request involves the removal of several GitHub Actions workflow files, including Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/manage-runs.yml (1)
Line range hint
35-48: Improve shell script robustnessThe current shell script has potential issues with word splitting and globbing. Let's make it more robust.
Apply this diff to improve the shell script:
- name: Cancel active workflow runs run: | - for id in ${WORKFLOW_IDS//,/ } + IFS=',' read -ra workflow_ids <<< "$WORKFLOW_IDS" + for id in "${workflow_ids[@]}" do - echo "Cancelling workflow with id: $id" + echo "Cancelling workflow with id: ${id}" # use curl here as I have no idea how to use a github action in a loop curl \ -X POST \ -H "Accept: application/vnd.github+json" \ -H "Authorization: Bearer ${{ github.token }}" \ - https://api.github.com/repos/${{ github.repository }}/actions/runs/$id/cancel + "https://api.github.com/repos/${{ github.repository }}/actions/runs/${id}/cancel" done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/manage-runs.yml(2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/manage-runs.yml
23-23: shellcheck reported issue in this script: SC2046:warning:11:21: Quote this to prevent word splitting
(shellcheck)
23-23: shellcheck reported issue in this script: SC2086:info:11:28: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: shellcheck reported issue in this script: SC2086:info:11:58: Double quote to prevent globbing and word splitting
(shellcheck)
23-23: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🔇 Additional comments (2)
.github/workflows/manage-runs.yml (2)
19-19: LGTM: Token usage follows best practices
The change to github.token aligns with GitHub's current best practices for accessing the default workflow token.
46-46: LGTM: Secure token usage in API call
The token is properly used in the Authorization header for the GitHub API call.
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/tests-build-js.yml (1)
Line range hint
17-17: Track the TODO as a separate issueThe comment indicates a planned migration from cache to upload artifacts action.
Would you like me to create a GitHub issue to track this TODO item for replacing the caching step with upload artifacts action?
.github/workflows/tests-rs-package.yml (2)
Line range hint
129-133: Track the FIXME as a separate issueThe comment indicates a limitation with PR-only execution of structure detection.
Would you like me to create a GitHub issue to track implementing support for manual triggers in the structure detection job?
Line range hint
144-190: Enhance error messages in structure detection scriptThe structure detection implementation is solid, but the error messages could be more descriptive.
Consider enhancing the error messages to include more context:
- echo "Deletions detected in @append_only structures in $file. Test failed." + echo "ERROR: Deletions detected in @append_only structures in $file. This violates the append-only constraint." - echo "Code changes detected in @immutable structures in $file. Test failed." + echo "ERROR: Code changes detected in @immutable structures in $file. This violates the immutability constraint."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/cached.yml(0 hunks).github/workflows/manage-runs.yml(0 hunks).github/workflows/prebuild-devcontainers.yml(1 hunks).github/workflows/tests-build-image.yml(1 hunks).github/workflows/tests-build-js.yml(1 hunks).github/workflows/tests-codeql.yml(1 hunks).github/workflows/tests-js-package.yml(2 hunks).github/workflows/tests-rs-package.yml(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/manage-runs.yml
- .github/workflows/cached.yml
🔇 Additional comments (10)
.github/workflows/prebuild-devcontainers.yml (1)
22-22: Timeout increase is appropriate for multi-arch container builds.
The increase to 240 minutes is reasonable given this job builds containers for both amd64 and arm64 architectures.
Let's verify the historical build times to confirm this timeout is appropriate:
.github/workflows/tests-codeql.yml (1)
8-8: Verify CodeQL analysis typically completes within 15 minutes.
While 15 minutes is generally sufficient for JavaScript CodeQL analysis, we should verify this against historical run times.
Let's check recent CodeQL job durations:
.github/workflows/tests-build-image.yml (3)
Line range hint 9-12: Good addition of the target parameter.
Adding the required target parameter improves workflow flexibility and aligns with multi-stage Docker build best practices.
Line range hint 1-1: Verify complete removal of manage-runs workflow.
While the timeout additions are good improvements, let's verify the primary PR objective of removing the manage-runs workflow.
Let's confirm the workflow removal:
✅ Verification successful
Workflow removal confirmed - no remaining references found
The verification confirms that:
- The
manage-runs.ymlworkflow file has been completely removed - No remaining references to "manage-runs" exist in any workflow files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify manage-runs workflow is removed
if [ -f .github/workflows/manage-runs.yml ]; then
echo "Warning: manage-runs.yml still exists"
else
echo "Confirmed: manage-runs.yml has been removed"
fi
# Check for any remaining references to the removed workflow
rg -l "manage-runs" .github/workflows/
Length of output: 182
21-21: Verify build time constraints for Docker images.
The 30-minute timeout should be sufficient for most Docker builds, but let's verify against historical data.
Let's check recent build durations:
.github/workflows/tests-build-js.yml (1)
8-8: LGTM: Reasonable timeout for JS build job
The 10-minute timeout is appropriate for a JS build job and aligns with timeouts in other workflows.
.github/workflows/tests-js-package.yml (3)
21-21: LGTM: Appropriate timeout for lint job
5-minute timeout is suitable for linting tasks.
56-56: LGTM: Reasonable timeout for test job
15-minute timeout is appropriate for test execution.
Line range hint 7-13: LGTM: Well-structured workflow inputs
The new inputs enhance workflow flexibility:
skip-tests: Allows conditional test executiontest-command: Enables customizable test commands
.github/workflows/tests-rs-package.yml (1)
127-127: LGTM: Appropriate timeout for structure detection
10-minute timeout is suitable for the structure detection job.
Issue being fixed or feature implemented
Manage PR workflow suppose to stop runs for closed PRs. This logic doesn't work well and flaky.
Debugging will take a lot of time. Since we switched to GitHub-hosted runners, running jobs for closed PRs it's not so big problem anymore.
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only