search

open-iscsi / targetd

Remote configuration of a LIO-based storage appliance
GNU General Public License v3.0
71 stars 28 forks source link

Expand API documentation for nfs_export_add #63

Open tasleson opened 4 years ago

tasleson commented 4 years ago

PR https://github.com/open-iscsi/targetd/pull/60 added return value to the API call nfs_export_add. Update documentation to reflect.

Also, before we cut the next release we should scrutinize this call and see if there is anything else we want to add to returned data.

tasleson commented 4 years ago

Related: https://github.com/open-iscsi/targetd/issues/62 :joy_cat:

dsonck92 commented 4 years ago

As for adding, considering that (probably) most will ignore extraneous fields, that might not be too important, however a consistent naming would be a must. It's much harder if an API suddenly changes the name which breaks clients expecting specific names (which I think is safe to assume nearly all do).

Train of thought I do not endorse: Technically if the return value is guaranteed to be a dict that may change but upon giving to a related function will perform the correct action, might allow clients to treat it as dynamic data. This would still allow UI's to show some feedback but obviously would limit any meaningfulness of the contained data. To visualize:

deletion_data = jsonrequest('nfs_export_add', options)
# Time passes
jsonrequest('nfs_export_delete', **deletion_data)
dsonck92 commented 4 years ago

Just as a note, I can pick this up if it's not done already (I don't believe it is but I might have missed stuff)

tasleson commented 4 years ago

Just as a note, I can pick this up if it's not done already (I don't believe it is but I might have missed stuff)

I've not done any work on this. I'll try to assign issues to myself before I work on them to avoid duplication of effort.

tasleson commented 4 years ago

@dsonck92 I think your approach of returning a dictionary is good. It should provide backwards compatibility while allowing us to add stuff moving forward.

dsonck92 commented 4 years ago

Meanwhile, at work, I did find one possible library that could be failing, Jackson. If someone decides to implement a client for Java and uses Jackson as serializer/deserializer for the json objects, by default, it will throw on unknown properties. That said, this can be worked around for them in two ways:

In the end, it boils down to documentation. I think, if we specify that it's possible fields will be added to the response, people that might use libraries which follow the Jackson approach of erroring when it's not exactly like you tell the library it is, can prepare for the issues. Either by asking their library of choice to ignore them, add them unconditionally in case new useful return values get added or purposely enabling it and do some integration testing so they sit on top of compatibility. Considering that, if the documentation always gets updated in tandem of api changes, it could even be done by them through a notification of the documentation file changing.

Anyways, I will see if I can add it this weekend, probably Sunday.