Fix value_specs implementation#2886
Conversation
17f6dc9 to
23b3306
Compare
| if port["value_specs"] != nil { | ||
| for k, v := range port["value_specs"].(map[string]interface{}) { | ||
| if slices.Contains(bannedKeys, k) { | ||
| return nil, errors.New(fmt.Sprintf("forbidden key in value_specs: %s", k)) | ||
| } | ||
| if _, ok := port[k]; ok { | ||
| return nil, errors.New(fmt.Sprintf("value_specs would overwrite key: %s", k)) | ||
| } | ||
| port[k] = v | ||
| } | ||
| delete(port, "value_specs") |
There was a problem hiding this comment.
I totally don't like the concept behind this and introducing Heat's mistakes into our API - Heat shouldn't be a standard we're following.
Despite of that - this code does it in an acceptable way, i.e. applications won't break when putting foo in value_specs and in new version foo is available directly.
23b3306 to
091f8e0
Compare
|
Did we ever return anything in the |
|
@lentzi90 doesn't Heat remove value_specs from the body before sending to neutron? shouldn't we do the same? |
Yes! I hope it is what I'm doing with delete(port, "value_specs")So first we take all keys under |
I don't think so |
|
Any way to re-trigger that one failed test besides pushing again? |
|
I have manually retriggered the run |
EmilienM
left a comment
There was a problem hiding this comment.
Minor change is required before we can merge, I think today.
091f8e0 to
8374114
Compare
8374114 to
8df0181
Compare
8df0181 to
3f3e49d
Compare
|
Thanks for the swift reviews! I think it should be in good order now 🙂 |
|
unit test job is broken but it's not your PR |
|
LGTM for me. I'll let a chance to other reviewers to have another look as well (cc @pierreprinetti). |
|
might need a rebase in fact |
The current implementation does not do anything useful. The key-pairs must be extracted and added directly in the body instead of under "value_specs". This commit fixes the implementation and also adds validation of the value_specs based on what is done in Heat. There are some banned keys that are not allowed in value_specs, and values are not allowed to overwrite existing fields either. Signed-off-by: Lennart Jern <lennart.jern@est.tech>
3f3e49d to
63e3079
Compare
|
I approve this change, I just want to see @dulek, @pierreprinetti and @mdbooth ack'ing it too. |
dulek
left a comment
There was a problem hiding this comment.
Looks okay from my perspective.
| // from the request body. It will return error if the value specs would overwrite | ||
| // an existing field or contains forbidden keys. | ||
| func AddValueSpecs(body map[string]interface{}) (map[string]interface{}, error) { | ||
| // Banned the same as in heat. See https://github.com/openstack/heat/blob/dd7319e373b88812cb18897f742b5196a07227ea/heat/engine/resources/openstack/neutron/neutron.py#L59 |
There was a problem hiding this comment.
Would probably be better to link to opendev.org instead of GitHub mirrors, no big deal.
There was a problem hiding this comment.
Ah my bad 🤦
Just say the word and I'll update...
The current implementation does not do anything useful. The key-pairs must be extracted and added directly in the body instead of under "value_specs".
This commit fixes the implementation and also adds validation of the value_specs based on what is done in Heat. There are some banned keys that are not allowed in value_specs, and values are not allowed to overwrite existing fields either.
Fixes #2870