Unhandled panic crash on rollout_history printer.PrintObj #130503

tahacodes · 2025-02-28T20:43:28Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR prevents a panic crash when user tries to output yaml, json, etc... on a revision history.
It also reflects the printer.PrintObj's error back to cobra so i would print the error message to the user instead of a panic stack trace

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1724

Special notes for your reviewer:

I've ran make verify, make test and make test-integration and i didn't find any issues related to this PR but if i messed things up or anything i'd love to follow-up more.

Does this PR introduce a user-facing change?

Fixed a panic issue related to kubectl revision history kubernetes/kubectl#1724

Signed-off-by: Taha Farahani <[email protected]>

linux-foundation-easycla · 2025-02-28T20:43:34Z

The committers listed above are authorized under a signed CLA.

✅ login: tahacodes / name: Taha Farahani (13bbbb6, 900f00d, 8a447ff, 20446af, 80eac9a, 4f5d0a0, 354d696, 1f8bb00, a8321a1, 26a909c)

k8s-ci-robot · 2025-02-28T20:43:37Z

Welcome @tahacodes!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-02-28T20:43:38Z

Hi @tahacodes. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tahacodes · 2025-02-28T21:01:12Z

/assign soltysh

tahacodes · 2025-02-28T21:04:03Z

Too soon :)

/unassign soltysh

googs1025 · 2025-03-01T11:23:37Z

/ok-to-test

Signed-off-by: Taha Farahani <[email protected]>

tahacodes · 2025-03-14T14:21:48Z

Also, can you write a unit test in rollout_history_test.go?

That will define the expected behavior when rollout history is called for a non-existent revision and also prevent a regression in the future.

I've pushed the new unit test and the other required change. @brianpursley

Instead of modifying the existing TestRolloutHistoryWithOutput to handle expected error conditions, I introduced a new test function called TestRolloutHistoryErrors. As discussed before, The original test relies on constructing the Cobra command, which makes it difficult to intercept os.Exit(1) or panics and compare them against the expected error message. Consequently, any failure within cmd.Run() would stop the test execution immediately.

In this new test, I directly call RolloutHistoryOptions.Run() to capture and validate the returned error. This allows the test to confirm if the panic occurs (as expected) and ensures that any mismatch in the error message will result in a test failure.

soltysh · 2025-04-07T14:56:02Z

/triage accepted
/priority important-longterm

soltysh · 2025-04-07T14:56:35Z

/label tide/merge-method-squash

soltysh

/lgtm
/approve

k8s-ci-robot · 2025-04-07T15:21:51Z

LGTM label has been added.

Git tree hash: 1fc06dddba1e4a6e3a7b2a9947a3cd6229c2bf05

k8s-ci-robot · 2025-04-07T15:21:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, tahacodes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2025-04-07T17:33:32Z

This PR will land for 1.34, and we'll want to backport this to 1.33, see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md for how to handle that bit

tahacodes · 2025-04-07T19:59:32Z

This PR will land for 1.34, and we'll want to backport this to 1.33, see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md for how to handle that bit

Thank you for the review. @soltysh
I think i got a little confused, i can't find the release-1.33 branch to cherry-pick to it which makes sense since it's not released yet.
Did you mean that This PR lands for 1.33 and i should backport it to 1.32?

soltysh · 2025-04-08T19:13:40Z

Apologies for being vague, below I explain what I mean what all that, I hope 😅

This PR will land for 1.34, and we'll want to backport this to 1.33, see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md for how to handle that bit

We're currently in code freeze for 1.33, which is scheduled to be released on April 23rd, 2025. This means only critical issues will land k8s 1.33. This particular PR was qualified as not being that, so it'll have to wait for the next release. It will start right after this release.

Thank you for the review. @soltysh I think i got a little confused, i can't find the release-1.33 branch to cherry-pick to it which makes sense since it's not released yet. Did you mean that This PR lands for 1.33 and i should backport it to 1.32?

Building on what I wrote above, we don't have release-1.33 branch yet, that will be created soon. So after the release this PR will be able to merge, and this will mean it'll be part of k8s 1.34. After that you'll be able to cherry-pick it into release-1.33 branch, which should be available by then.

Hope that clear things out :)

…story printer.PrintObj (#131496) * Change: Handling nil runtime.Object Signed-off-by: Taha Farahani <[email protected]> * Change: Return only if there is error in rollout_history Signed-off-by: Taha Farahani <[email protected]> * Change: Return the unknown revision error directly in rollout_history.go Signed-off-by: Taha Farahani <[email protected]> * Change: Remove unintended newline Signed-off-by: Taha Farahani <[email protected]> * Change: Using go idiomatic way for checking if historyInfo[o.Revision] exists Signed-off-by: Taha Farahani <[email protected]> * Change: Remove 'error:' from returned error message in rollout_history.go Signed-off-by: Taha Farahani <[email protected]> * Change: Check for printer.PrintObj returned err Signed-off-by: Taha Farahani <[email protected]> * Change: Add TestRolloutHistoryErrors test Signed-off-by: Taha Farahani <[email protected]> * Change: Simple typo fix on Complete() function description Signed-off-by: Taha Farahani <[email protected]> * Change: Checking for error on o.Complete in TestRolloutHistoryErrors Signed-off-by: Taha Farahani <[email protected]> --------- Signed-off-by: Taha Farahani <[email protected]>

…#130503) * Change: Handling nil runtime.Object Signed-off-by: Taha Farahani <[email protected]> * Change: Return only if there is error in rollout_history Signed-off-by: Taha Farahani <[email protected]> * Change: Return the unknown revision error directly in rollout_history.go Signed-off-by: Taha Farahani <[email protected]> * Change: Remove unintended newline Signed-off-by: Taha Farahani <[email protected]> * Change: Using go idiomatic way for checking if historyInfo[o.Revision] exists Signed-off-by: Taha Farahani <[email protected]> * Change: Remove 'error:' from returned error message in rollout_history.go Signed-off-by: Taha Farahani <[email protected]> * Change: Check for printer.PrintObj returned err Signed-off-by: Taha Farahani <[email protected]> * Change: Add TestRolloutHistoryErrors test Signed-off-by: Taha Farahani <[email protected]> * Change: Simple typo fix on Complete() function description Signed-off-by: Taha Farahani <[email protected]> * Change: Checking for error on o.Complete in TestRolloutHistoryErrors Signed-off-by: Taha Farahani <[email protected]> --------- Signed-off-by: Taha Farahani <[email protected]>

Change: Handling nil runtime.Object

13bbbb6

Signed-off-by: Taha Farahani <[email protected]>

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 28, 2025

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 28, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 28, 2025

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 28, 2025

github-project-automation bot added this to SIG CLI Feb 28, 2025

github-project-automation bot moved this to Needs Triage in SIG CLI Feb 28, 2025

k8s-ci-robot requested review from ardaguclu and brianpursley February 28, 2025 20:43

k8s-ci-robot assigned soltysh Feb 28, 2025

k8s-ci-robot unassigned soltysh Feb 28, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 14, 2025

tahacodes added 2 commits March 14, 2025 17:18

Change: Simple typo fix on Complete() function description

354d696

Signed-off-by: Taha Farahani <[email protected]>

Change: Checking for error on o.Complete in TestRolloutHistoryErrors

26a909c

Signed-off-by: Taha Farahani <[email protected]>

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 7, 2025

soltysh approved these changes Apr 7, 2025

View reviewed changes

k8s-ci-robot assigned soltysh Apr 7, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2025

k8s-ci-robot merged commit 609e4a9 into kubernetes:master Apr 23, 2025
14 checks passed

k8s-ci-robot added this to the v1.34 milestone Apr 23, 2025

github-project-automation bot moved this from In Progress to Done in SIG CLI Apr 23, 2025

tahacodes mentioned this pull request Apr 27, 2025

Automated cherry pick of #130503: Unhandled panic crash on rollout_history printer.PrintObj #131496

Merged

tahacodes deleted the rollout_history_printobj_panic branch May 14, 2025 21:02

This was referenced May 18, 2025

Bump kubectl from v1.30.4 to v1.33.1 kokyhm/kubespray#211

Open

Bump kubeadm from v1.30.4 to v1.33.1 kokyhm/kubespray#212

Open

Bump kubelet from v1.30.4 to v1.33.1 kokyhm/kubespray#214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unhandled panic crash on rollout_history printer.PrintObj #130503

Unhandled panic crash on rollout_history printer.PrintObj #130503

tahacodes commented Feb 28, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 28, 2025 •

edited

Loading

k8s-ci-robot commented Feb 28, 2025

k8s-ci-robot commented Feb 28, 2025

tahacodes commented Feb 28, 2025

tahacodes commented Feb 28, 2025

googs1025 commented Mar 1, 2025

tahacodes commented Mar 14, 2025 •

edited

Loading

soltysh commented Apr 7, 2025

soltysh commented Apr 7, 2025

soltysh left a comment

k8s-ci-robot commented Apr 7, 2025

k8s-ci-robot commented Apr 7, 2025

soltysh commented Apr 7, 2025

tahacodes commented Apr 7, 2025

soltysh commented Apr 8, 2025

Unhandled panic crash on rollout_history printer.PrintObj #130503

Unhandled panic crash on rollout_history printer.PrintObj #130503

Conversation

tahacodes commented Feb 28, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Feb 28, 2025 • edited Loading

k8s-ci-robot commented Feb 28, 2025

k8s-ci-robot commented Feb 28, 2025

tahacodes commented Feb 28, 2025

tahacodes commented Feb 28, 2025

googs1025 commented Mar 1, 2025

tahacodes commented Mar 14, 2025 • edited Loading

soltysh commented Apr 7, 2025

soltysh commented Apr 7, 2025

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 7, 2025

k8s-ci-robot commented Apr 7, 2025

soltysh commented Apr 7, 2025

tahacodes commented Apr 7, 2025

soltysh commented Apr 8, 2025

tahacodes commented Feb 28, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 28, 2025 •

edited

Loading

tahacodes commented Mar 14, 2025 •

edited

Loading