Conversation
…tion The cli-proxy image collection in docker.go duplicated the logic from isCliProxyNeeded() in compiler_difc_proxy.go. Replace the inline check with the existing function to eliminate the duplication and ensure both codepaths stay in sync when the proxy enablement logic changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Hey One minor note:
If you'd like a hand addressing this, you can assign this prompt to your coding agent:
|
There was a problem hiding this comment.
Pull request overview
Refactors collectDockerImages to delegate CLI proxy enablement logic to the existing isCliProxyNeeded() helper, reducing duplicated conditional logic and keeping CLI-proxy image pulling aligned with step generation behavior.
Changes:
- Replaced inline
cli-proxy/integrity-reactionsfeature-flag checks in docker image collection with a singleisCliProxyNeeded(workflowData)call. - Simplified the related comment block in
docker.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/docker.go | Uses isCliProxyNeeded() to decide whether to include the cli-proxy AWF sidecar image in the pre-pull list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
| // Add cli-proxy sidecar container when the cli-proxy is needed. | ||
| // Without this, --skip-pull causes AWF to fail because the cli-proxy image was never pulled. | ||
| if isCliProxyNeeded(workflowData) { |
There was a problem hiding this comment.
collectDockerImages now relies on isCliProxyNeeded(), but docker_cli_proxy_test.go doesn’t currently cover the implicit enablement path via features.integrity-reactions: true. Adding a test case that asserts the cli-proxy image is included when integrity-reactions is enabled (and excluded when AWF is too old / firewall disabled) would protect this refactor from regressions and verify the image-collection path stays aligned with step generation.
| if cliProxyNeeded && awfSupportsCliProxy(firewallConfig) { | ||
| // Add cli-proxy sidecar container when the cli-proxy is needed. | ||
| // Without this, --skip-pull causes AWF to fail because the cli-proxy image was never pulled. | ||
| if isCliProxyNeeded(workflowData) { |
There was a problem hiding this comment.
isCliProxyNeeded(workflowData) re-checks isFirewallEnabled() and re-derives firewallConfig internally, even though this block already established firewall is enabled and already computed firewallConfig/awfImageTag. This adds redundant work and can emit difc_proxy debug logs from the image-collection path; consider adding a helper that accepts the already-computed firewallConfig (or splitting feature-flag evaluation from firewall/version gating) so docker image collection doesn’t repeat checks.
This PR simplifies recently modified code from PR #26154 to improve clarity and maintainability while preserving all functionality.
Files Simplified
pkg/workflow/docker.go— replaced duplicated inline cli-proxy check with the existingisCliProxyNeeded()functionImprovement
PR #26154 added
isCliProxyNeeded()incompiler_difc_proxy.goto encapsulate the logic for when the CLI proxy should be started (checking both the explicitcli-proxyfeature flag and the implicitintegrity-reactionstrigger, plus firewall and version checks).However,
docker.gostill had its own inline version of the first part of that logic:Replaced with a single call to the authoritative function:
This eliminates the duplication and ensures that if the proxy enablement conditions ever change, both the step generation and the image collection paths update together.
Changes Based On
Testing
TestCollectDockerImages_CliProxy,TestIsCliProxyNeeded_IntegrityReactionsImplicitEnable)make build)Review Focus
Please verify:
isCliProxyNeeded()without behavioral differencesAutomated by Code Simplifier Agent — analyzing code from the last 24 hours