Small tweaks to Rust code after a review pass #5
Conversation
Adds `Debug`, `Clone`, `Eq` and `PartialEq` to the `AppConfig`.
* Reduce duplication in error mapping. * Use StatusCode::BAD_REQUEST for all error cases.
The `result` bindings can be inlined.
Adds `Debug`, `Eq`, `PartialEq`.
This binding wasn't being used for anything and can be inlined.
This lets the caller use the call to `start_and_supervise_queue_processing` as the result from `new` directly.
This avoids OpenSSL + pkg-config dev-dependencies.
The unit tests for the Rust app require cg-diff be in PATH.
I suspect it might be useful to debug the event, job_id, and output dir.
Rather than converting the finalized MAC tag to a byte slice and comparing with standard equality check, use `mac.verify_slice`, a constant time comparison. This removes the ability to debug the computed vs actual tag but I think in practice this won't be super useful and avoiding timing side channels is more important.
Previously the `handle_issue_comment` logic used the GitHub login string of the user posting the comment to decide if it was a comment from the bot. This requires hardcoding the name of the application, which may be fragile. Instead, include the ID from the GitHub user model and in `handle_issue_comment` check if it matches the configured application GitHub application ID.
| @@ -75,7 +75,7 @@ impl EventQueue { | |||
| octocrab.clone(), | |||
| ); | |||
|
|
|||
| info!("background task started for event queue"); | |||
| info!("background task started for event queue job {active_job_id:?}"); | |||
There was a problem hiding this comment.
I'd expect the active job's id here won't provide meaningful information (it gets reset on every loop iteration and will always be None at the beginning of the loop). If the goal is to log the job id when a job starts we should do it in process_queued_events_in_background. Is that what you meant?
There was a problem hiding this comment.
If the goal is to log the job id when a job starts we should do it in process_queued_events_in_background. Is that what you meant?
That sounds more useful :-) I think I wasn't thinking about it very deeply. I'll put up a fix.
|
This is perfect! There is only one change that I'm not sure about (see this comment), but I'm going to merge right away and we can sort it out later. |
I've done a review pass through all of the Rust code. This branch contains a handful of small fixes/tweaks I made along the way since I couldn't leave review feedback on already merged code :-)
Feel free to treat these changes as if they were suggestions. I'm happy to drop any that aren't to your taste. Probably best reviewed commit-by-commit.
One extra note: I was going to add Nightly coverage to CI but
sqlxand the current nightly are interacting badly due to a rustc bug. Happy to revisit that once the upstream issues are fixed.