baremetal: add network_data to Node interface#2154
baremetal: add network_data to Node interface#2154jtopjian merged 1 commit intogophercloud:masterfrom
Conversation
|
Build failed.
|
|
recheck |
|
Build failed.
|
|
Opened #2155 to report the (pre-existing) test failure. |
This field was added in microversion 1.66 and appears in the Victoria release. Depends-On: theopenlab/openlab-zuul-jobs#1147
2c7f4fa to
d59284b
Compare
|
Build failed.
|
| Owner string `json:"owner,omitempty"` | ||
|
|
||
| // Static network configuration to use during deployment and cleaning. | ||
| NetworkData map[string]interface{} `json:"network_data,omitempty"` |
There was a problem hiding this comment.
To confirm: this is just an arbitrary/generic map/dict/hash that will accept any data?
Normally if a field was added from a microversion, it should be a pointer so that the field is omitted from the boddy if it isn't set. But it looks like Ironic is accounting for having this field set when the microversion is less than 1.35: https://github.com/openstack/ironic/blob/462969c68f2978b4b250ba99ef491ef6858437fe/ironic/objects/node.py#L554
Because of this, I'm okay with leaving it as map[string]interface{}, but if you wanted to change it to *map[string]interface{}, go for it.
There was a problem hiding this comment.
To confirm: this is just an arbitrary/generic map/dict/hash that will accept any data?
For these purposes, I think we have to treat it as that. Ironic does do json schema validation on it, by default to check that it matches the Nova network_data.json format. However, that format could theoretically change over time, and there's also a config option to allow the operator to specify a different schema (which is how I actually want to use this).
Normally if a field was added from a microversion, it should be a pointer so that the field is omitted from the boddy if it isn't set. But it looks like Ironic is accounting for having this field set when the microversion is less than 1.35: https://github.com/openstack/ironic/blob/462969c68f2978b4b250ba99ef491ef6858437fe/ironic/objects/node.py#L554
Because of this, I'm okay with leaving it as
map[string]interface{}, but if you wanted to change it to*map[string]interface{}, go for it.
If you're OK with it I think I'd prefer to leave it, because there's no useful distinction between the field being empty vs. not set.
There was a problem hiding this comment.
If you're OK with it I think I'd prefer to leave it, because there's no useful distinction between the field being empty vs. not set.
In this case, I agree. Usually it's because the API will return an error for an invalid field when the microversion isn't set correctly.
jtopjian
left a comment
There was a problem hiding this comment.
LGTM. I think this is safe to merge even with the failing OpenLab tests.
This field was added in microversion 1.66 and appears in the Victoria release.
For #2153
Links to the line numbers/files in the OpenStack source code that support the
code in this PR:
https://opendev.org/openstack/ironic/src/branch/master/ironic/api/controllers/v1/node.py#L161-L165
https://review.opendev.org/c/openstack/ironic/+/687910/