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

Refactors apiserver errors to be module specific #279

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

jpmcb
Copy link
Contributor

@jpmcb jpmcb commented Oct 7, 2022

Issue number:

Related: #121

Description of changes:

Refactors the apiserver error handling to be less generic and more module spcific.

Testing done:

Coming soon

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@jpmcb jpmcb requested a review from gthao313 October 7, 2022 22:52
#[snafu(display("Unable to create client: '{}'", source))]
ClientCreate { source: kube::Error },

pub enum ApiError {
Copy link
Member

Choose a reason for hiding this comment

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

Do you have preferences and any thoughts that make you keep this error.rs? I think might be better to delete this file and move the ApiError module to mod.rs? I'm not 100% confident that my way is better than yours. Love to hear your thoughts! thanks

Copy link
Contributor Author

@jpmcb jpmcb Oct 10, 2022

Choose a reason for hiding this comment

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

This made sense to live in it's own file since it denotes errors for the entire API module. I believe putting it in mod.rs would essentially accomplish the same thing; make these errors accessible to entire module (but not necessarily the crate). Still new to the rust paradigms, so up to what y'all think!

apiserver/src/api/drain.rs Outdated Show resolved Hide resolved
#[snafu(display("Unable to create client: '{}'", source))]
ClientCreate { source: kube::Error },

pub enum ApiError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout our codebase (including other projects) we tend to use Error for all error enums instead of giving them bespoke names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I was following some of the other examples found in brupop:

pub enum AuthorizationError {

Maybe these are due for a small refactor?

Signed-off-by: John McBride <jpmmcb@amazon.com>
@jpmcb
Copy link
Contributor Author

jpmcb commented Oct 10, 2022

Force pushed:

  • Resolved formatting with cargo fmt
  • ApiError --> Error

@jpmcb jpmcb merged commit 6dcf070 into bottlerocket-os:develop Oct 11, 2022
@jpmcb jpmcb deleted the apiserver-error-refactor branch October 11, 2022 17:07
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.

3 participants