Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Append final errors to error array and treat request errors as retryable #11

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

bretthoerner
Copy link
Contributor

I've been working on the janitor and noticed 2 issues:

  1. We don't append the error on fail when it seems like we should.
  2. We were treating request errors as different from RetryableWebhookError, but we want things like connection errors, timeouts etc to be RetryableWebhookError AFAIK

.await?;
.await
.map_err(|e| WebhookConsumerError::RetryableWebhookError {
reason: e.to_string(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we surface Webhook send errors to users, I think we'll want to tame this with an enum rather than leaving it open ended, but that seems unrelated to this change and I'm leaving it for later.

Copy link
Contributor

@tomasfarias tomasfarias left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you

Comment on lines +45 to +46
.build()
.expect("failed to construct reqwest client for webhook consumer");
Copy link
Contributor

@tomasfarias tomasfarias Dec 15, 2023

Choose a reason for hiding this comment

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

There is nothing fallible in this method if we make this change (as we'll panic), so we can return Self instead of a Result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye! I feel like clippy or something should tell me that. 🤔

@bretthoerner bretthoerner merged commit 988a1b0 into main Dec 15, 2023
4 checks passed
@bretthoerner bretthoerner deleted the brett/err branch January 17, 2024 15:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants