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

commands: add get audit events command #243

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

joe-prosser
Copy link
Contributor

Adds a command to return audit events

@joe-prosser joe-prosser self-assigned this Nov 16, 2023
Copy link
Collaborator

@tommilligan tommilligan left a comment

Choose a reason for hiding this comment

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

LGTM, plus it compiles

impl Iterator for QueryResponseIterator {
type Item = PrintableAuditEvent;
fn next(&mut self) -> Option<Self::Item> {
let response = self.response.audit_events.get(self.index).map(|event| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid nesting here, I would return early if you get None like:

Suggested change
let response = self.response.audit_events.get(self.index).map(|event| {
let response = self.response.audit_events.get(self.index)?;
let actor_email = ...

Comment on lines 136 to 140
let actor_email = &self
.response
.get_user(&event.actor_user_id)
.unwrap_or_else(|| panic!("Could not find user for id `{}`", event.actor_user_id.0))
.email;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If perf is ever an issue here, I would suggest converting the raw response into some hashmap lookups for users/datasets etc. at the point you convert it to QueryResponseIterator, so you're not repeatedly scanning with find.

But happy this works.

Note that I think it is technically possible for us to not return all linked resources (e.g. if the user has been deleted, won't be included).

I'm happy for your cases this is not likely, but if you were pushing this out to other users, I'd want some nicer error handling here.

@joe-prosser joe-prosser merged commit 374b6f7 into master Nov 16, 2023
2 checks passed
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