🐛 (CLI): Fix redundant guard and swallowed checkout error in alpha update push#5694
Conversation
…pha update push Remove a redundant nested if opts.Push guard and check the git checkout error that was previously discarded with _ in the alpha update push path. The inner if opts.Push on line 209 is always true because the outer guard on line 208 already checks the same field. This is a copy-paste artifact. The git checkout error on line 211 is silently discarded with _. While the checkout is typically a no-op (the squash path already leaves the user on the output branch), silently discarding the error means any systemic git failure (corrupted state, locked index) is hidden from the user, who sees Update completed successfully despite the error. The error check pattern matches the identical checkout handling at lines 197-198 for the ShowCommits path. Introduced in kubernetes-sigs#5013 (2025-08-18). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @SebTardif. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
|
||
| // Push the output branch if requested | ||
| if opts.Push { | ||
| if opts.Push { |
There was a problem hiding this comment.
The command can work locally and not necessary we want to push the new branch
Therefore, we must to keep the option here
/hold
There was a problem hiding this comment.
Thanks for the review! Just to clarify: the outer if opts.Push guard on line 208 is preserved, so the push remains fully optional. The only change is removing the duplicate inner if opts.Push that was nested inside it (always true, since the outer block already gates on the same condition), and surfacing the checkout error that was silently discarded with _.
The push behavior is unchanged: when Push is false, none of this code runs.
k8s-ci-robot
added
the
do-not-merge/hold
|
@camilamacedo86 The The checkout still only runs inside |
|
I see good catcher !!! |
|
/hold cancel /approved /lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, SebTardif The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of the kubebuilder alpha update push flow by removing a redundant opts.Push guard and ensuring git checkout failures are surfaced to the user instead of being silently ignored.
Changes:
- Removed a redundant nested
if opts.Pushblock in the push path. - Propagated
git checkout <output-branch>errors before attemptinggit push. - Added integration tests covering successful push behavior and checkout-failure error propagation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/cli/alpha/internal/update/update.go | Removes redundant guard and returns a user-visible error when output-branch checkout fails before pushing. |
| internal/cli/alpha/internal/update/update_test.go | Adds integration coverage for the push path (success case and checkout failure propagation). |
|
/retest-required |
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What
Remove a redundant nested
if opts.Pushguard and check the git checkout error that was previously discarded with_in thealpha updatepush path.Why
Two issues in the push block of
alpha update:The inner
if opts.Pushon line 209 is always true because the outer guard on line 208 already checks the same field. This is a copy-paste artifact.The git checkout error on line 211 is silently discarded with
_. While the checkout is typically a no-op (the squash path already leaves the user on the output branch), silently discarding the error means any systemic git failure (corrupted state, locked index) is hidden from the user, who sees "Update completed successfully" despite the error.The error check pattern matches the identical checkout handling at lines 197-198 for the
ShowCommitspath.How
if opts.Pushblock_ = helpers.GitCmd(...)toif err := helpers.GitCmd(...); err != nil { return ... }Introduced in #5013 (2025-08-18).