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

Implement Webhook cleanup #15

Merged
merged 15 commits into from
Dec 21, 2023
Merged

Implement Webhook cleanup #15

merged 15 commits into from
Dec 21, 2023

Conversation

bretthoerner
Copy link
Contributor

@bretthoerner bretthoerner commented Dec 19, 2023

No description provided.

@bretthoerner bretthoerner marked this pull request as ready for review December 20, 2023 22:42
@bretthoerner
Copy link
Contributor Author

I got Kafka mocking working and finished up the tests, so this is ready (again).

Ok(category)
}

fn serialize_error_type<S>(error_type: &Option<ErrorType>, serializer: S) -> Result<S::Ok, S::Error>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I may be wrong but I think there is a Serialize/Deserialize implementation for Option and &Option, so we could implement Serialize/Deserialize on ErrorType instead and we would get Serialize/Deserialize on &Option<ErrorType> too. Not a big deal, but would save the check for Some(error_type)/None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, maybe I need a special serde annotation to make it work?

error[E0308]: mismatched types
   --> hook-common/src/kafka_messages/app_metrics.rs:46:23
    |
46  | #[derive(Deserialize, Serialize, Debug, PartialEq, Clone)]
    |                       ^^^^^^^^^ expected `ErrorType`, found `&Option<ErrorType>`
...
67  |         serialize_with = "serialize_error_type",
    |                          ---------------------- arguments to this function are incorrect
    |
    = note:   expected enum `ErrorType`
            found reference `&'__a std::option::Option<ErrorType>`
note: function defined here
   --> hook-common/src/kafka_messages/app_metrics.rs:120:4
    |
120 | fn serialize_error_type<S>(error_type: ErrorType, serializer: S) -> Result<S::Ok, S::Error>
    |    ^^^^^^^^^^^^^^^^^^^^    ---------------------
    = note: this error originates in the derive macro `Serialize` (in Nightly builds, run with -Z macro-backtrace for more info)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a little searching but I don't know if this is possible, if you know a way I'm happy to come back and clean it up.


const APP_METRICS_TOPIC: &str = "app_metrics";

async fn create_mock_kafka() -> (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! We may want to re-use this in other tests, so I'm keeping it in mind.

Comment on lines +752 to +755
// We have committed, what remains are:
// * The 1 available job we completed while the txn was open.
// * The 2 brand new jobs we added while the txn was open.
// * The 1 running job that didn't change.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

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.

Looks pretty good to me, some minor nitty comments here and there that do not necessarily need addressing right now. Thanks!

@bretthoerner bretthoerner merged commit 980765d into main Dec 21, 2023
4 checks passed
@bretthoerner bretthoerner deleted the brett/webhook-cleanup branch December 21, 2023 17:09
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