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

Fixes #26244: License repository errors should result in different exit codes #6137

Open
wants to merge 1 commit into
base: branches/rudder/8.3
Choose a base branch
from

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Jan 22, 2025

https://issues.rudder.io/issues/26244

The main can return an ExitCode, we only have to map it from the result of running the CLI : we intercept the repository errors and associate specific error codes depending on the HTTP error. For now the exit code are 2 and 3.

@clarktsiory clarktsiory requested a review from amousset January 22, 2025 16:26
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

Copy link
Member

@amousset amousset left a comment

Choose a reason for hiding this comment

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

👍 for the return code handling and the usage of a custom error type!

This is good on paper, but we will actually almost never reach a 403. The way the authentication code works is by hiding the existence of unauthorized files, due to technical reasons, so the 403 and 404 are mostly indistinguishable.
Additionally, the 403 error very likely means the credentials are valid but the content forbidden, so the error message is not right.
The original code does a pretty bad job at error reporting!

Furthermore, I think the RepositoryError should not expose raw HTTP semantics but rather a "Rudder-level" error and keep the matching private.

Maybe something like:

#[derive(Debug)]
pub enum RepositoryError {
    InvalidCredentials(anyhow::Error),
    Unauthorized(anyhow::Error),
}

While keeping the details in the error itself with something like:

let e = anyhow!("Invalid credentials, please check your credentials in the configuration. (received HTTP {status}");
RepositoryError::InvalidCredentials(e)

let e = anyhow!("Unauthorized dowload from Rudder reporsitory. (received HTTP {status}");
RepositoryError::Unauthorized(e)

@clarktsiory clarktsiory force-pushed the ust_26244/license_repository_errors_should_result_in_different_exit_codes branch from 8f13b1b to dd0dbc5 Compare January 23, 2025 10:52
@clarktsiory
Copy link
Contributor Author

PR updated, the changes have been made !
Now there is one error code per enum case (so there are only 2 of them)

@clarktsiory
Copy link
Contributor Author

PR rebased

@clarktsiory clarktsiory force-pushed the ust_26244/license_repository_errors_should_result_in_different_exit_codes branch from dd0dbc5 to a8dfdb0 Compare January 23, 2025 11:01
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