Deduplicate AllowedLabels into a shared embedded safe-output config struct - #50988
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Deduplicates AllowedLabels through a shared embedded configuration while preserving existing YAML behavior.
Changes:
- Adds
SafeOutputAllowedLabelsConfigand embeds it in shared filters. - Replaces five duplicate declarations.
- Updates affected test literals.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_parser.go |
Defines shared allowed-label configuration. |
pkg/workflow/create_issue.go |
Embeds shared configuration. |
pkg/workflow/create_pull_request.go |
Embeds shared configuration. |
pkg/workflow/create_discussion.go |
Embeds shared configuration. |
pkg/workflow/update_discussion.go |
Embeds shared configuration. |
pkg/workflow/merge_pull_request.go |
Preserves deprecated alias through embedding. |
pkg/workflow/safe_outputs_tools_test.go |
Updates test literals. |
pkg/workflow/safe_outputs_config_generation_test.go |
Updates generation test literals. |
pkg/workflow/compiler_threat_detection_formal_test.go |
Updates validation test literals. |
pkg/workflow/compiler_safe_outputs_config_test.go |
Updates compiler test literals. |
pkg/cli/compile_workflow_processor_test.go |
Updates CLI test literals. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot Please triage this PR, refresh the branch if needed, and run the Branch refresh was requested. No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Run context: https://github.com/github/gh-aw/actions/runs/31148473746
|
4a45496 is still the relevant head for this request. The branch is already refreshed against |
|
@copilot Please triage this PR, refresh the branch if needed, and run the No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state, summarize any remaining blocker keeping the PR in blocked status, and hand back a concise maintainer-ready summary. Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31154408939
|
PR TriageCategory: refactor (dedupe AllowedLabels into shared config struct) · Risk: medium · Score: 52/100 (impact 20, urgency 12, quality 20) Recommendation: Consolidates
|
c057be5 is the current head. The branch is already refreshed against |
|
@copilot Please address the remaining review feedback and run the Open review items to address:
Branch refresh was requested.
|
c057be5 is maintainer-ready from my side. The branch is already refreshed against |
|
@copilot Please address the remaining review feedback and run the Open review items to address:
Branch refresh was requested.
|
c057be5 is still maintainer-ready from my side. The branch is already refreshed against |
|
@copilot Please triage this PR, refresh the branch if needed, and run the No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31173041415
|
c057be5 is maintainer-ready from my side.
Actions taken: verified current PR review state, inspected latest branch workflow runs/logs, reran the full local validation suite, and confirmed no repo changes were introduced in this pass. Hand-off: CI must be re-triggered by a maintainer if fresh GitHub signals are required before merge. |
|
@copilot Please triage this PR, refresh the branch if needed, and run the No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Branch refresh was requested.
|
c057be5 is still maintainer-ready from my side.
Actions taken: rechecked current PR/reviewer state, reconfirmed the branch head and clean local status, and re-inspected the latest branch workflow/check state for actionable failures. Hand-off: CI must be re-triggered by a maintainer if fresh GitHub signals are required before merge. |
|
@copilot Please triage this PR, refresh the branch if needed, and run the No failed checks are currently reported on the latest head SHA. Please confirm reviewer feedback state and hand back a concise maintainer-ready summary. Branch refresh was requested.
|
c057be5 is still maintainer-ready from my side.
Actions taken: rechecked current PR/reviewer state, reconfirmed the branch head and clean local status, and re-inspected the latest branch workflow/check state for actionable failures. Hand-off: CI must be re-triggered by a maintainer if fresh GitHub signals are required before merge. |
|
🎉 This pull request is included in a new release. Release: |
AllowedLabels []stringwas declared identically in 5 safe-output config structs inpkg/workflow, despite shared embeddable filter structs already existing insafe_outputs_parser.go.Changes
New shared struct in
pkg/workflow/safe_outputs_parser.goholding the single declaration, embedded inline intoSafeOutputFilterConfig:Removed the 5 duplicate declarations, embedding the shared struct instead in
CreateIssuesConfig,CreatePullRequestsConfig,CreateDiscussionsConfig,UpdateDiscussionsConfig, andMergePullRequestConfig(whereallowed-labelsremains a deprecated alias forrequired-labels).Test struct literals updated to the embedded form. All read/write sites (
config.AllowedLabels) are untouched — field promotion keeps them valid.Why a dedicated struct rather than embedding
SafeOutputFilterConfigThe issue suggested embedding
SafeOutputFilterConfigdirectly into the 5 structs. That struct also declarestitle-prefix, which has different semantics in the create-* configs — there it's the prefix applied to created titles, not a filter. Direct embedding would produce a duplicatetitle-prefixyaml key and silently change parsing. The nested one-field embed satisfies the same goal (declared once, available onSafeOutputFilterConfig) while leaving each config's yaml surface unchanged.Worth confirming during review: no struct embeds both
SafeOutputFilterConfigandSafeOutputAllowedLabelsConfig, which would create an ambiguous selector and duplicate yaml key. A grep overpkg/shows none currently do.