-
Notifications
You must be signed in to change notification settings - Fork 61
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
Re-work API to support full object modifications #92
Comments
if you start and provide simple one object changing to new interface we can check it. I'm try sometimes ago to do something usable in this pr #59 but not have time to finish. And as i saw create all options blows code |
@alexanderConstantinescu Thanks for bringing this up. Currently the APIs are modeled close to what ovn-nbctl provides, and missing parts were added gradually on-demand, and some of them are still missing, such as setting options for routers, as you mentioned. I agree that in some cases it is better to support full object operations with all the properties, but at this point it is ok to combine multiple API calls into a single transaction to execute, if we are able to add the missing parts. In the longer term I would expect to have a code generator similar to what C IDL does, to generate all the CRUD APIs according to schema. However, this is not a priority yet. Before this is ready, if you want to add new APIs manually that takes full object as input, I am totally ok with it, but we need to figure out a good naming convention, to distinguish from existing CRUD APIs (I'd avoid replacing existing APIs, but just adding new ones). Or, would it work for you by just adding the missing part of "Set" APIs, e.g. to set the options for LR? |
Yes, to not break compatibility we would definitely need to either add individual "setters" or add new methods allowing full object modification. I will try to look into doing this, given the time available to me. But for anyone following along: in case I don't post any updates here or file a PR, feel free to pick this task up. |
Currently this library supports modifying a small aspect of all OVN objects, which makes it quite unusable in certain use cases. Ex:
Logical Routers in OVN currently have these fields:
Now, these fields are internally modifiable and supported by the library as we can see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L26
However, the CRUD operations exposed by the API do not allow the modification of most of these fields, as we see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L40
This means that a user of this library can effectively only set
external_ids
on any Logical Router, and moreover: also renders some functional operations completely impossible, such as settingoptions
on the Logical Router.I would expect to see the API allow the following operations
and
Depending on the time available to me, I might file a PR with this. But I would like to get some input on if these changes are valid. They directly modify the API, so it would be an important change.
The text was updated successfully, but these errors were encountered: