xapi-project / xapi-storage

Experimental new storage interface for xapi
https://xapi-project.github.io/xapi-storage/
9 stars 19 forks source link

CA-283728: put generated code into new namespace #73

Closed edwintorok closed 6 years ago

edwintorok commented 6 years ago

The already shipped supp-pack with the tmpfs SR imports a python library from the installed host. If we update the host without updating the supp-pack we need to keep the python library backward compatible.

The short-term fix is to keep the old library as was, and put the new library into a different module. In the future the code generator can be updated to generate a single python module that is compatible with all the APIs.

Need to be merged together with: https://github.com/xapi-project/xapi-storage-script/pull/57 (and the internal PR on the spec files)

mseri commented 6 years ago

The errors are:

edwintorok commented 6 years ago

I think these are all from the auto-generated code, I'll try to fix the code generator and regenerate it than manually fix the output.

mseri commented 6 years ago

Thanks! I think we can live without the aesthetic fixes, but the comparisons need to be fixed

mseri commented 6 years ago

Let’s run pyflake with —ignore E201,E202,E301,E501 And fix the remaining redefinition warnings

edwintorok commented 6 years ago

Still testing the latest fixup, will post an update when its ready for another round review.

edwintorok commented 6 years ago

Should be better now, the _test class is not actually used, thought we could use them in xapi-storage-script's unit tests, but there are several things to fix first before it'd work (e.g. Plugin.query test class returns a json object wrapped in a query_result field, whereas it should just return the inner dict), so I think we should do that as part of the RPC rework to support multiple APIs.

mseri commented 6 years ago

Should we delete it for the time being? (it probably would be enough to comment the code that generates that part until it's fixed)

edwintorok commented 6 years ago

Not sure whether anything else uses it somewhere, I'd keep it for now.

mseri commented 6 years ago

If it's broken I wonder how whatever uses it is not failing miserably

mseri commented 6 years ago

Feel free to squash the fixups and merge, please open a CA ticket about this broken test class

mseri commented 6 years ago

Unless you preemptively set it differently, the logging module defaults to warning level and will discard all the logging.info calls

edwintorok commented 6 years ago

Sorry, missed this comment earlier, I used logging.basicConfig with loglevel set to debug when attempting to write a unit test, before I hit CA-284338.

edwintorok commented 6 years ago

Rebased and squashed the fixups, checked that there were no differences in the code except history: git diff d96112b..fc4355e