fix: handle deployments without command in patcher#7706
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the Patcher component by adding support for Kubernetes deployments that do not explicitly define a container command. By intelligently choosing between updating the Command or Args fields, the patcher now avoids errors and correctly preserves image entrypoints while still applying necessary configuration updates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
karmada-bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the deployment patcher to support patching container arguments (Args) when the container command (Command) is empty, and adds a corresponding unit test. A review comment points out a critical issue where subcommands or non-flag arguments in Args could be silently dropped during parsing and overwriting, and provides a code suggestion to preserve them.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if useArgs { | ||
| container.Args = command | ||
| } else { |
There was a problem hiding this comment.
When useArgs is true, if the first element of container.Args is a subcommand or a non-flag argument (e.g., ["run", "--config=..."]), it will fail parseArgument validation and be ignored during parsing. Consequently, when container.Args is overwritten with command, this subcommand will be silently dropped, which can break the deployment.
To prevent this, we should check if the first element of baseArguments is a non-flag argument (by verifying if parseArgument returns an error) and preserve it by prepending it to the generated command list, similar to how we preserve the binary name in Command.
if useArgs {
if len(baseArguments) > 0 {
if _, _, err := parseArgument(baseArguments[0]); err != nil {
command = append([]string{baseArguments[0]}, command...)
}
}
container.Args = command
} else {There was a problem hiding this comment.
Pull request overview
Hardens Patcher.ForDeployment so it no longer panics when patching a Deployment whose first container relies on the image entrypoint (empty Command). The patcher now falls back to patching Args in that case, and a regression test is added.
Changes:
- Update
ForDeploymentto patchArgswhenCommandis empty, while preserving existing behavior whenCommandis present. - Add a table-driven regression test covering the empty
Commandscenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| operator/pkg/util/patcher/pather.go | Adds Command vs Args selection logic when merging extraArgs/featureGates for Deployments. |
| operator/pkg/util/patcher/pather_test.go | Adds a regression case for Deployments with empty Command to ensure patching doesn’t panic and writes flags into Args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useArgs := len(baseArguments) == 0 | ||
| if useArgs { | ||
| baseArguments = container.Args | ||
| } | ||
| argsMap := parseArgumentListToMap(baseArguments) |
| Containers: []corev1.Container{ | ||
| { | ||
| Name: "test-container", | ||
| Image: "nginx:latest", | ||
| }, |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7706 +/- ##
=======================================
Coverage 42.06% 42.06%
=======================================
Files 879 879
Lines 54831 54832 +1
=======================================
+ Hits 23063 23067 +4
+ Misses 30023 30021 -2
+ Partials 1745 1744 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Elvand-Lie <elvandlie@gmail.com>
Elvand-Lie
force-pushed
the
fix-patcher-empty-command
branch
from
ee6fc4c to
d07fcdb
Compare
What type of PR is this?
/kind bug
What this PR does / why we need it:
Patcher.ForDeploymentcurrently assumes the first container has a non-emptyCommandwhen applyingextraArgsorfeatureGates, and indexesbaseArguments[0]unconditionally.This PR hardens that path for deployments that rely on the image entrypoint and leave
Commandempty. WhenCommandis present, the patcher keeps the existing behavior and preserves the first command element as the binary name. WhenCommandis empty, the command-argument merge is skipped so the patcher does not panic and does not reinterpret arbitrary KubernetesArgs.A regression case is added to the existing
TestPatchForDeploymenttable.Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Local checks run:
go test ./operator/pkg/util/patcher -run 'TestPatchForDeployment/PatchForDeployment_WithExtraArgsAndEmptyCommand_Patched' -count=1 -vgo test ./operator/pkg/util/patcher -count=1gofmt -l operator/pkg/util/patcher/pather.go operator/pkg/util/patcher/pather_test.gogit diff --checkmingw32-make testis not usable in this Windows shell; it fails before Go tests atmkdir -p ./_output/coverage/. A previoushack/verify-all.shrun in MSYS bash failed on unrelatedpkg/util/lifted/doc.gogenerated-doc drift.Does this PR introduce a user-facing change?: