-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add python bindings for hwlib #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few inline suggestion, overall very clear README/code, thank you
client/README.md
Outdated
|
||
* `0`: The system has not been seen (default behaviour). | ||
* `1`: The system is partially certified (we haven't seen this specific system, but some of its hardware components have been tested on other systems). | ||
* `2`: This system has been certified (but probably for other Ubuntu release). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly dumb question, could 2 be "System is certified for another ubuntu version" and 3 "System is certified for this ubuntu version"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! However, we don’t currently differentiate these states in the API server implementation. @mz2 should we add this state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "certified state exists for system" kind of response should cover both conditions of "system is certified for another ubuntu version" and "system is certified for this ubuntu version but your system differs from it". A better explanation for it would be "there exists a certified state for this system, you're presently not on it. Here's the delta: ..." (delta being supplied in form of the itemised, structured response including the state of the certified state(s) for a system like it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're proposing including multiple "partially happy" scenarios as examples that could be toggled with this env state, that indeed does sound sensible in follow-up PRs once there exists some basic client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks pretty good. One comment below. Also, I know it's just example data for now, but since we are adding python bindings, it would be good to go ahead and create a tests directory and at least add a simple python test to ensure it can be called from python. That gives us a good place to add additional tests to later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, merge it, keep in mind the possible followup with the status for exact match
cf3e155
to
b29eebd
Compare
6f77ade
to
783520d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I look to have left my review in draft. Fine for all this to be addressed in follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot better now, thanks for the tests! both the rust and the python tests work for me, as well as getting the expected results running things interactively.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff, thank you very much for the Python tests and all the effort.
|
||
match response { | ||
Ok(response_struct) => { | ||
let json_str = serde_json::to_string(&response_struct).map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unnecessary dance done in this implementation to first parse, then JSON stringify, then parse again (with a different, slower JSON parser, and parsing a different JSON than what came from the HTTP response since the stringify is done on Serde parsing output).
The runtime cost of this is not that problematic because the payload is predictably quite small, but there's a reliability concern: it's possible for one of the JSON parsing methods to fail, and for the Serde types to be different from the OpenAPI schema in some way. All of this we can wrangle in follow-up PRs, just noting this here for posterity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, thanks for adding tests, and I'm happy the async/await change was this simple to accomplish.
There is a follow-up needed at some later point in making the data available to the Python bindings: right now the data is parsed twice (serde's output is essentially thrown away whereas those structs could directly be exposed).
This PR adds the ability to use
hwlib
functions in the Python code.I've also documented how to install and use is in the client README
P.S. don't judge too strictly, this is my second Rust contribution :)