Conversation
|
Build succeeded.
|
jtopjian
left a comment
There was a problem hiding this comment.
@stbenjam thanks!
This is a really good start. Our rule has been for a PR to only implement 1 method (eg GET or POST or DELETE). But I've been thinking of changing that rule when someone is implementing a new API suite. It just gets too tedious the other way around.
The code here is really clean, so if you'd like to keep this PR as only CRUD, I'm good with that. Supplemental methods (perhaps what you've categories as Node Management) should be individual small PRs. See some of the closed Senlin/Clustering PRs as examples.
In addition, there will need to be some further investigation work done with the Service API code links. https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/node.py is a valid file, but where in that file can I tell that all fields in ListOpts or CreateOpts are valid? The contributor guide will have example PRs/Issues with the kinds of detailed references that make reviewing this easier. Not only does it save me hours of time grepping Ironic's code, but the process very often finds bugs or undocumented ways that the API works - these situations become enhanced when using a language like Go.
I've left some comments in-line. Let me know if you have any questions. Thank you again!
|
Build succeeded.
|
|
Thanks for taking the time to review my PR. This should be in better shape for review. I've added some basic acceptance testing - is there Ironic available in the environment there? I've also added links to the relevant Ironic code for create and list. I do think the API would accept some additional fields in |
|
Also what should I do about the coveralls failures? I think things like my addition to the service client is causing the decrease in coverage: https://coveralls.io/builds/21468077/source?filename=service_client.go I could add some unit tests to cover the SC. |
You can safely ignore coveralls :)
No, OpenLab doesn't support Ironic at this time. Including acceptance tests and verifying they've worked in your own environment is good enough. Thank you for making all of the requested changes! I will have time to review this in detail today or tomorrow. |
|
Build succeeded.
|
jtopjian
left a comment
There was a problem hiding this comment.
@stbenjam This is a really nice, clean implementation - thank you! I'm not familiar with Ironic nor can I test it, but the code looks good to me.
I've noted a few nits with field names. Once those are fixed, this is good to go.
|
Great, thank you so much for the reviews! I've updated the acronyms to be uppercase - I'll be more careful of that on future PR's. |
|
Build succeeded.
|
|
Hi! Is there any code to provide/detect microversions here? This patch adds a lot of features that are only available with a microversion. |
|
@dtantsur That's a good point to bring up. Microversions are really, really hard to work with in Go. There are a handful of areas where they are already being implemented. Usually this involves placing the responsibility of the developer using Gophercloud to make sure microversion-added features are used correctly (ex: if a field is set, make sure the client has the relevant microversion set). We have some rough guidelines on how to handle microversions here: https://github.com/gophercloud/gophercloud/blob/master/docs/MICROVERSIONS.md Let me know if this changes anything with how this PR should be structured. I'll hold off on merging for now. |
|
Not sure what you're looking for exactly, but yes this PR provides the ability to set the Ironic microversion by setting it on the service clients, that's shown in the acceptance tests for example. All the various fields are marked omitempty if not specified. That's basically the sum of microversion support in gophercloud at the moment until the proposals in #441 become a reality. |
Just a side note, the doc I posted is the result of those discussions. It was merged a few days ago and I forgot to close that issue. But thank you so much for reading through all of that history :) |
|
@stbenjam To confirm, I think you're saying that you've kept microversions in mind as best as possible? If so, This all LGTM and I'll merge it. |
|
@jtopjian I think so. I've tried my changes on multiple Ironic versions and they work, even if they don't know about certain fields (e.g. @dtantsur would know better than I if there needs to be any other special handling other than that. |
|
OK, sounds good. I'll leave this open for a little. |
dtantsur
left a comment
There was a problem hiding this comment.
I think there is an issue with removing attributes in PATCH. Also some minor comments inline.
|
@dtantsur Thanks for looking, responses above. I also pushed fixes for the docs, missing owner, and |
|
Build succeeded.
|
|
@dtantsur Could you look again please? I would like this to get in soon so I can open some additional pull requests for what we need in metalkube. |
|
Build succeeded.
|
|
I think this should be ready to go, let me know if you'd like me to make any additional changes. Thanks! |
jtopjian
left a comment
There was a problem hiding this comment.
LGTM - really nice work
|
Thanks!!! |
For #1429
First PR to add support for Ironic to gophercloud. This contains the basics for node support. Node management API will be done separately, along with the other API objects as listed in #1429.
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
General:
Create:
Nodeclass definition codeList: