-
-
Notifications
You must be signed in to change notification settings - Fork 638
Add /run-skipped-tests as alias for /run-skipped-ci (#XXXX) #2068
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
Conversation
This change adds /run-skipped-tests as a shorter, more intuitive alias for the /run-skipped-ci GitHub PR comment command. Changes: - Updated run-skipped-ci.yml to accept both /run-skipped-ci and /run-skipped-tests - Updated detect-invalid-ci-commands.yml to recognize /run-skipped-tests as valid - Updated help message to document the alias This fixes the issue where /run-skipped-tests would not trigger any response, when users expected it to either work or show help. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces a new command alias Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
Code Review - PR #2068: Add /run-skipped-tests alias✅ Overall AssessmentThis is a well-implemented, low-risk improvement that adds a more intuitive command alias. The changes are focused, consistent, and properly integrated across both workflows. 🎯 Strengths
📋 Code QualityWorkflow Trigger Logic (.github/workflows/run-skipped-ci.yml:18-22)
Help Detection Exclusions (.github/workflows/detect-invalid-ci-commands.yml:20-22)
Command Validation (.github/workflows/detect-invalid-ci-commands.yml:46)
Documentation (.github/workflows/detect-invalid-ci-commands.yml:82-98)
|
The workflow map had incorrect filenames that don't exist: - 'main.yml' -> should be 'integration-tests.yml' - 'pro-package-tests.yml' -> should be 'pro-test-package-and-gem.yml' This was causing workflow_dispatch to fail with "Workflow does not have 'workflow_dispatch' trigger" errors because it couldn't find the files. Also added 'Rspec test for gem' (gem-tests.yml) to the map for completeness. All workflows in the map have been verified to have workflow_dispatch triggers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Feedback✅ Overall AssessmentThis is a well-implemented improvement that adds a more intuitive command alias. The changes are clean, consistent, and properly integrated across both workflow files. 👍 What's Done Well
📋 Documentation Updates NeededThe PR changes the workflow files but does not update the documentation files that reference these commands:
Recommendation: Update both documentation files to mention the new alias, similar to how the help message in 🔍 Minor Observations
✅ Security & Best Practices
🧪 Testing VerificationThe PR description mentions manual testing was done:
Recommendation: These tests look comprehensive. Consider documenting this test procedure in 📝 Action Items Before MergeRequired:
Optional: 🎯 SummaryThis is a solid UX improvement with clean implementation. The main issue is missing documentation updates to match the workflow changes. Once the documentation is updated, this will be ready to merge. Estimated effort to address: ~5 minutes to update the two markdown files. 🤖 Review generated by Claude Code |
Updated all references to /run-skipped-ci in documentation to mention the new /run-skipped-tests alias: - CONTRIBUTING.md: Updated command header and example - .github/read-me.md: Updated 6 references throughout the file - Command header - When to use section - Security section - Concurrency section - Utility workflows list - Conditional execution section This ensures users can discover and use either command interchangeably. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2068SummaryThis PR adds ✅ Strengths1. Excellent Problem Identification
2. Comprehensive Implementation
3. Important Bug Fix in Commit 2The workflow filename corrections are critical: - 'Main test': 'main.yml',
+ 'Integration Tests': 'integration-tests.yml',
+ 'Rspec test for gem': 'gem-tests.yml',
- 'React on Rails Pro - Package Tests': 'pro-package-tests.yml',
+ 'React on Rails Pro - Package Tests': 'pro-test-package-and-gem.yml',This fixes silent failures when triggering workflows via 4. Proper Exclusion LogicThe \!contains(github.event.comment.body, '/run-skipped-tests')This prevents false positives in the invalid command detector. 🔍 Observations & Suggestions1. Workflow Verification ✅I verified all workflow files in the map have
2. Pattern ConsistencyThe trigger conditions use both startsWith(github.event.comment.body, '/run-skipped-tests') ||
contains(github.event.comment.body, '\n/run-skipped-tests')This handles both commands at the start of comments and after newlines. Good! 👍 3. Minor: Command OrderingIn the help text and valid commands list, the alias is consistently shown second. This is a good UX pattern - it clearly indicates which is the primary command and which is the alias. 4. Testing CoverageThe PR description mentions manual testing verification, which is appropriate for workflow changes. Consider adding this to future PRs:
🔒 Security & Access Control
🎯 Performance Considerations
🧪 Test CoverageThis change is to GitHub Actions workflow YAML files, which don't have traditional unit tests. However:
Recommendation: After merge, verify in a test PR that:
📚 Documentation Quality
🐛 Potential IssuesNone found. The implementation is clean and follows GitHub Actions best practices. 📋 Checklist Against CLAUDE.md Guidelines
🎉 Overall AssessmentRecommendation: ✅ APPROVE This is a high-quality PR that:
Impact: Low-risk, high-value change. Users get a more intuitive command, and the workflow map bug fix prevents silent failures. Special kudos for catching and fixing the workflow filename issues in commit 2 - that would have caused 💡 Minor Enhancement Suggestion (optional, not blocking)Consider adding a note to # Verify workflow file exists and has workflow_dispatch trigger
gh workflow view integration-tests.ymlThis could help prevent the filename issues that were fixed in commit 2. Great work! This PR is ready to merge. 🚀 |
## Summary Adds `/run-skipped-tests` as a shorter, more intuitive alias for the `/run-skipped-ci` GitHub PR comment command. ## Problem When users tried `/run-skipped-tests` in PR comments, nothing happened - no workflow triggered and no help message appeared. This was confusing because: 1. The command seemed like it should work (it's more intuitive than `/run-skipped-ci`) 2. The help detection workflow should have caught it and displayed available commands 3. Users were left wondering if the command was broken or if they did something wrong ## Root Cause The `/run-skipped-tests` command wasn't recognized as either valid or invalid, so it fell into a gap where no workflow would trigger. ## Changes 1. **Updated `run-skipped-ci.yml`**: Added `/run-skipped-tests` as an alias that triggers the same workflow 2. **Updated `detect-invalid-ci-commands.yml`**: - Added `/run-skipped-tests` to the valid commands list - Added it to the exclusion filter to prevent false "invalid command" detection - Updated help message to document the alias ## Testing Verified the logic works correctly: - `/run-skipped-tests` now triggers the run-skipped-ci workflow ✅ - `/run-skipped-tests` does NOT trigger the help message (it's valid) ✅ - `/run-skipped-ci` still works as before ✅ - Invalid commands like `/run-foo` still trigger the help message ✅ ## Impact - Users can now use the more intuitive `/run-skipped-tests` command - The existing `/run-skipped-ci` command continues to work - Help messages now document both options 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `/run-skipped-tests` command alias as an alternative way to trigger the skipped CI workflow. * **Documentation** * Updated help text and examples to document both `/run-skipped-ci` and `/run-skipped-tests` command options. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <noreply@anthropic.com>
…se-otp-timing * origin/master: (27 commits) Fix doctor command false version mismatch for beta/prerelease versions (#2064) Fix beta/RC version handling in generator (#2066) Document Rails Engine development nuances and add tests for automatic rake task loading (#2067) Add /run-skipped-tests as alias for /run-skipped-ci (#XXXX) (#2068) Fix: Add Rails 5.2-6.0 compatibility for compact_blank (#2058) Break CI circular dependency with non-docs change (#2065) Fix CI safety check to evaluate latest workflow attempt (#2062) Fix yalc publish (#2054) Add Shakapacker 9.0+ private_output_path integration for server bundles (#2028) Consolidate all beta versions into v16.2.0.beta.10 (#2057) Improve reliability of CI debugging scripts (#2056) Clarify monorepo changelog structure in documentation (#2055) Bump version to 16.2.0.beta.10 Bump version to 16.2.0.beta.9 Fix duplicate rake task execution by removing explicit task loading (#2052) Simplify precompile hook and restore Pro dummy app to async loading (#2053) Add Shakapacker precompile hook with ReScript support to Pro dummy app (#1977) Guard master docs-only pushes and ensure full CI runs (#2042) Refactor: Extract JS dependency management into shared module (#2051) Add workflow to detect invalid CI command attempts (#2037) ... # Conflicts: # rakelib/release.rake
Summary
Adds
/run-skipped-testsas a shorter, more intuitive alias for the/run-skipped-ciGitHub PR comment command.Problem
When users tried
/run-skipped-testsin PR comments, nothing happened - no workflow triggered and no help message appeared. This was confusing because:/run-skipped-ci)Root Cause
The
/run-skipped-testscommand wasn't recognized as either valid or invalid, so it fell into a gap where no workflow would trigger.Changes
run-skipped-ci.yml: Added/run-skipped-testsas an alias that triggers the same workflowdetect-invalid-ci-commands.yml:/run-skipped-teststo the valid commands listTesting
Verified the logic works correctly:
/run-skipped-testsnow triggers the run-skipped-ci workflow ✅/run-skipped-testsdoes NOT trigger the help message (it's valid) ✅/run-skipped-cistill works as before ✅/run-foostill trigger the help message ✅Impact
/run-skipped-testscommand/run-skipped-cicommand continues to work🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/run-skipped-testscommand alias as an alternative way to trigger the skipped CI workflow.Documentation
/run-skipped-ciand/run-skipped-testscommand options.✏️ Tip: You can customize this high-level summary in your review settings.