Skip to content

[code-simplifier] refactor: use isCliProxyNeeded() in docker.go to remove logic duplication #26168

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.

Sign up for GitHub

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

Merged
pelikhan merged 1 commit into from
Apr 14, 2026
+3 −7
Merged

[code-simplifier] refactor: use isCliProxyNeeded() in docker.go to remove logic duplication #26168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions pkg/workflow/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,9 @@ func collectDockerImages(tools map[string]any, workflowData *WorkflowData, actio
}
}

// Add cli-proxy sidecar container when the cli-proxy is needed (explicitly via
// cli-proxy feature flag, or implicitly via integrity-reactions feature flag)
// and the AWF version supports it. Without this, --skip-pull causes AWF to fail
// because the cli-proxy image was never pulled.
cliProxyNeeded := isFeatureEnabled(constants.CliProxyFeatureFlag, workflowData) ||
isFeatureEnabled(constants.IntegrityReactionsFeatureFlag, workflowData)
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) {
Comment on lines +106 to +108
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
cliProxyImage := constants.DefaultFirewallRegistry + "/cli-proxy:" + awfImageTag
if !imageSet[cliProxyImage] {
images = append(images, cliProxyImage)
Expand Down
Loading