Skip to content

Conversation

@imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Apr 24, 2025

The Rust part of the project was introduced just as a command-line interface. However, it has turned into a full blown HTTP/API server (with a command-line) and we plan to move more code from Ruby to Rust.

We are learning Rust as we go, so some old decisions might need to be revisited. I have started by trying to simplify the ServiceError enum, which represents an error but includes variants for many different situation (even unrelated ones). We even have an "anyhow" variant!

This first round includes:

  • Defining an error for each HTTP client.
  • Defining an error for each store.
  • Defining an error for the storage, which includes one variant for each store.
  • Remove some ServiceError variants.
  • Make use of anyhow in more places of the binary.

Once we have put everything into a better place, we can decide to simplify the whole thing if needed. Let's go step by step.

To do

Follow-up

  • There is another whole category errors that are related to D-Bus. Let's move them to their own error and finish the ServiceError clean-up.
  • The HTTP client constructors do not need to be async.
  • The D-Bus clients are the "special cases". Perhaps we should consider moving from DomainHTTPClient to DomainClient (and DomainClient to DomainDBusClient).

Warning

Do not merge before #2270. We need to fix some conflicts first.

@imobachgs imobachgs changed the title refactor(rust): reduce ServiceError (part 1) refactor(rust): reduce ServiceError (round 1) Apr 24, 2025
#[derive(Debug, thiserror::Error)]
pub enum BootloaderHTTPClientError {
#[error(transparent)]
HTTP(#[from] BaseHTTPClientError),
Copy link
Contributor

Choose a reason for hiding this comment

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

using transparent for base looks good to me, I would just probably use Base as enum key instead of HTTP as it is confusing with identical enum vealue from BaseHTTPClientError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variant actually wraps BaseHTTPClientError which contains HTTP-related errors (get, post, etc.). The idea is that other errors are more domain-specific.


#[derive(Debug, thiserror::Error)]
pub enum StoreError {
#[error(transparent)]
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to have it transparent here? I think additional line can help with seeing which part of store failed

Copy link
Contributor Author

@imobachgs imobachgs Apr 24, 2025

Choose a reason for hiding this comment

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

Each error already contain a description (e.g., "Error processing storage settings: {0}").

@imobachgs imobachgs requested a review from jreidinger April 25, 2025 10:51
@imobachgs imobachgs merged commit 9cd3d93 into master Apr 25, 2025
7 checks passed
@imobachgs imobachgs deleted the refactor-service-errors branch April 25, 2025 12:34
@imobachgs imobachgs mentioned this pull request May 2, 2025
@imobachgs imobachgs mentioned this pull request May 26, 2025
imobachgs added a commit that referenced this pull request May 26, 2025
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