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

Update pyauditor to version 0.0.7 #286

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

stefan-k
Copy link
Contributor

Updates pyauditor to the most recent version 0.0.7. Fixes #283.

@giffels giffels requested review from a team, giffels and eileen-kuehn and removed request for a team February 20, 2023 14:07
Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the updating the code. I have one unrelated comment,

pyauditor.Meta()
.insert("site_id", [resource_attributes["site_name"]])
.insert("user_id", [self._user])
.insert("group_id", [self._group])

looks a bit weird. Using singular meta data identifiers, but inserting lists into it. Is there any reason for this?

@stefan-k
Copy link
Contributor Author

looks a bit weird. Using singular meta data identifiers, but inserting lists into it. Is there any reason for this?

The Meta object is effectively a hash map where a key (string) maps to a list of values (strings). I expect that at some point, someone is going to ask about how to store multiple values per key, hence I thought it might be a good idea to add this functionality right away. For instance, one potential use case would be storing the nodes a multi-node job was running on.

I haven't tried it yet, but I suspect passing a single element instead of a list causes complaints of Rust's type system (or more precisely the wrapper, since everything's compiled already). However, for the next version I'll see what I can do to make this a bit more pythonic :)

@giffels
Copy link
Member

giffels commented Feb 20, 2023

Thanks for the explanation.

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 👍

@giffels giffels added this pull request to the merge queue Feb 23, 2023
Merged via the queue into MatterMiners:master with commit 7abce1f Feb 23, 2023
@stefan-k stefan-k deleted the pyauditor_0.0.7 branch February 23, 2023 14:36
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.

Unittests of the Auditor Plugin are failing
3 participants