Open Stack

Hi Steve:

Thanks for your great efforts for WSME replacement.

I had a quick review for your code changes, see my comments below:

Review
        Feedback
        Reason

704490
        Has concern
        move wsme code to ironic, types.py

704487
        Has concern
        move wsme code to ironic, node.py

704489
        has concern
        move wsme code to ironic, args.py

704486
        has concern
        move wsme code to ironic expose.py

703898
        Both positive and concern
        Positive, better code structure than original code, concern, more difficult to locate WSME code

704488
        has concern
        move wsme code to ironic

704485
        has concern
        Add pecan code, pecan would be replaced by flask

703897
        Positive
        Removed some WSME code with this change

703723
        Positive
        Removed some WSME code with this change

703695
        Positive
        Removed some WSME code with this change


Firstly, I appreciated 3 changes which definately removed some wsme code, especially the change 703897.

But I have some concerns for changes 704490, 704487, 704489, ...,  these changes seemed move WSME code into ironic, that would make ironic code base become a bit large, from my personal view, to use python built-in feature or other 3-party lib to replace WSME function would be better.  I like the way that code change 703897 did.


Thanks
Jerry

发自我的iPhone

在 2020年1月30日，上午8:53，Steve Baker <sbaker at redhat.com> 写道：

I've put together a set of changes for removing WSME which involves copying just enough of it into ironic, and I think we need to have the conversation about whether this approach is desirable :) This git branch[1] finishes with WSME removed and existing tests passing.

Here are some stats about lines of code (not including unit tests, calculated with cloc):

4500 wsme/wsme

6000 ironic/ironic/api master

7000 ironic/ironic/api story/1651346

In words, we need 1000 out of 4500 lines of WSME source in order to support 6000 lines of ironic specific API code.

Switching to a replacement for WSME would likely touch a good proportion of that 6000 lines of ironic specific API code. If we eventually replace it with a new library I think it would be easier if the thing being replaced is inside the ironic source tree to allow for a gradual transition. So this approach could be an end in itself, or it could be a step towards the final goal.

My strategy for copying in code was to pull in chunks of required logic while removing some unused features (like request pass-through and date & time type serialization). I also replaced py2/3 patterns with pure py3. There is likely further things which can be removed or refactored for simplicity, but what exists currently works.

If there is enough positive feedback for this approach I'll start on unit test coverage for the new code.

[1] https://review.opendev.org/#/q/topic:story/1651346


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20200130/b7f1d2a7/attachment-0001.html>