Skip to content
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.

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

Handle and print error instead of panic when passing nonexistent context #51

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

nicmr
Copy link
Collaborator

@nicmr nicmr commented Dec 8, 2023

Description

Currently, when trying to pass a context that does not exist in the configured KUBECONFIG, e.g kc foo or kubesess context -v foo, the program panics:

❯ kubesess context -v foo
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/commands.rs:96:19
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This doesn't tell the user what went wrong and makes what is just a user error look like a bug.

To fix this, set_context now returns a Result<(), SetContextError>, which is appropriately handled. If there is an error, the program will exit with a non-zero exit code and report the error on stderr (similarly to the error handling in commands::selectable_list). The kubeconfig variable will not be changed (because of the non-zero exit code).

Fixed behaviour:

❯ kc foo
Error: Couldn't find context foo

Implementation hints / Considerations

  • I used an enum with a single case as the error type to make it extensible. If you'd prefer, a struct could be used instead. Overall, it doesn't really make a big difference.
  • A dedicated error crate like thiserror could make the error implementation more terse, but that adds another depedency that is not needed at this point. It's only 12 lines as is.

Looking forward to your feedback :)
Also thanks for creating kubesess, I use it everyday and it makes my life easier, particularly with direnv it's a charm

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • End-to-end test using kubesess and the provided shell scripts
  • Not tested using the fish scripts

Test Configuration:

  • Firmware version (does not make sense to me, listing kernel): Linux 6.6.4-arch1-1
  • Hardware: Intel i7-1260P
  • Toolchain: stable-x86_64-unknown-linux-gnu
  • SDK: rustc 1.70.0

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Ramilito
Copy link
Owner

Ramilito commented Dec 8, 2023

Hi and welcome!

Also thanks for creating kubesess, I use it everyday and it makes my life easier, particularly with direnv it's a charm

That makes me super happy to hear! I'm using it every day as well so this improvement is a really good idea!

I used an enum with a single case as the error type to make it extensible. If you'd prefer, a struct could be used instead. Overall, it doesn't really make a big difference.

Since it can only be one error type at a time I think this is the correct choice 👍

Looking forward to your feedback :)

I think this is a great addition and addresses something I haven't noticed 😅, I mostly use the select list and the backwards history, so big thank you for this PR!

I will approve it since it's super nice by itself but you got me thinking about how to scale this, using the enum as is right now would scale great for context error but less for the modes, namespace or config reading, so if you don't mind, I might extract it to a separate file and make the enum name more generic while keeping them specific to the error to replace all eprint that we use today and add elsewhere, Or do you prefer that everyone creates their own custom errors?

Also, what would you think of having a similar error message as kubectl config use-context non-existing? (it's: error: no context exists with the name: "non-existing")

@Ramilito
Copy link
Owner

Ramilito commented Dec 8, 2023

You can ignore the test not working, I think it's bugged for PR:s

@nicmr
Copy link
Collaborator Author

nicmr commented Dec 9, 2023

using the enum as is right now would scale great for context error but less for the modes, namespace or config reading, so if you don't mind, I might extract it to a separate file and make the enum name more generic while keeping them specific to the error to replace all eprint that we use today and add elsewhere, Or do you prefer that everyone creates their own custom errors?

Yes, don't hesitate to adapt it to the structure that makes sense for your future plans. Having it in a separate error.rs module is a really common pattern.

If I understand your questions correctly, you're weighing using one "global" Error enum vs. specific enums for the different modes?
From my experience, I usually start with a single Error enum - it's easy, fast to implement and if modes encounter similar errors, it prevents duplication.
If the error type eventually grows too big and functions end up not having that much overlap, and e.g. you find yourself meaningfully handling only a handful of error variants in each place and just providing a blanket match for the rest because they will never actually be thrown at that location, that's probably the point where it makes sense to refactor the error types into several enums.
Also once the error handling gets a bit more sophisticated I can really recommend the thiserror crate, it makes wrapping external error types super straightforward and saves you from implementing Display (and std:error::Error) by hand over and over.

Also, what would you think of having a similar error message as kubectl config use-context non-existing? (it's: error: no context exists with the name: "non-existing")

Great idea, this will be the most familiar for users 👍

  • I'm going to update the display implementation.

You can ignore the test not working, I think it's bugged for PR:s

I think it's because the pull_request trigger does not allow external PRs to access the repo secrets to prevent malicious PRs from stealing them. The pull_request_target trigger does allow for secret access, but will run without your approval iirc. Maybe it could check for an "allow-tests" label and only execute once you add that label to PRs
if: ${{ github.event.label.name == 'allow-tests' }}

Or, assuming the contents of the secret are not actually confidential, the test kubeconfig could be provided inline or as a repository variable (reference) instead?

@Ramilito Ramilito merged commit 97d68e0 into Ramilito:main Dec 11, 2023
@Ramilito
Copy link
Owner

Right, that makes sense, removing the secret in those jobs should be enough probably, it's just as you guessed a dummy value.

I haven't tried thiserror yet, did enjoy using error_context crate the other day but will try thiserror out and compare!

BTW! If you have any ideas on new features or changes in existing ones please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants