-
Notifications
You must be signed in to change notification settings - Fork 443
Support ready_for_review for pull_request_target triggers
#41161
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1410,6 +1410,7 @@ | |
| "reopened", | ||
| "synchronize", | ||
| "converted_to_draft", | ||
| "ready_for_review", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] 💡 Suggested description updateAt line 1399, change: "description": "List of pull request target event types to trigger on"to mirror "description": "Pull request target event types to trigger on. Note: converted_to_draft and ready_for_review represent state transitions rather than states. While technically valid to listen for both, consider if you need to handle both transitions or just one."And add the "$comment": "converted_to_draft and ready_for_review are logically opposite state transitions. Using both may indicate unclear intent."This keeps both event definitions consistent and surfaces the guidance to tooling that renders schema descriptions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incomplete parity fix: 💡 DetailsThe Users who write: on:
pull_request_target:
types: [milestoned]will still get a schema validation error after this PR merges. Add |
||
| "locked", | ||
| "unlocked", | ||
| "enqueued", | ||
|
|
||
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.
[/tdd] The new test verifies
ready_for_reviewin isolation, but the PR description highlights the motivating real-world pattern astypes: [opened, ready_for_review]. A combination test would better capture that scenario and guard against any future combinatorial validation bugs.💡 Suggested additional test case
{ name: "valid pull_request_target combined types trigger", frontmatter: map[string]any{ "on": map[string]any{ "pull_request_target": map[string]any{ "types": []any{"opened", "ready_for_review"}, }, }, "engine": "claude", }, filePath: "/test/workflow.md", wantErr: false, },This mirrors the example in the PR description and ensures the enum validation works correctly when multiple types are combined.