_order_steps() method of Workflow is broken

[Dmitry Mescheryakov]

The method is here:
https://github.com/openstack/horizon/blob/grizzly-2/horizon/workflows/base.py#L619

The method sorts steps which were dynamically added to workflow with register(...). The steps are compared using their 'before' and 'after' fields. The proper way to compare such elements having "incomplete" order is topological sorting. The method instead employs an incorrect greedy algorithm. The method can fail if input list has size of 3 or more.

I see two ways to fix the issue:

1. (preferred) remove sorting and simply show tabs in the order they were registered. That makes code simpler - we get rid of 'after' and 'before' fields of Step and _order_steps() method of Workflow. On the other hand seems like nobody uses that feature, otherwise they would already bump into the issue.

2. Implement proper sorting.

I can provide a fix for that issue

[Ladislav Smola (lsmola)]

First I saw the contribute/depend on logic. I have expected some sort of intelligent sorting of steps, that depends on workflow's contribute/depends on fields, seems it's not there.

It will be good to raise a verbose exception, that will say that you need to reorder your code, if the order is bad. Like when one step that depends on previous step's data is placed badly.

So I guess 3. option is: let the steps be in the order they were registered, with verbose exception if it is ordered badly (depending on contribute/depends on logic)

What do you think?

[Dmitry Mescheryakov (dmitrymex)]

Ladislav,

Such check is easy to implement, but frankly I don't see much value in it. I just can't imagine the case where order of registered Steps is not obvious from source code.

Our usecase is that we want to register Steps in Workflow constructor. I.e. different instances of our Workflow class might have different Steps. I don't see such mechanism implemented in Workflow at the moment.

Initially we thought to use 'register(...)' method for that (and that is how I found that issue with incorrect sorting). But we found that 'register(...)' adds Step to Workflow class instead of instance. That is strange because actually to register such a 'static' Steps, one can simply append the Step to Workflow.default_steps. So that 'register(...)' seems to be redundant.

I wonder for what purpose 'register(...)' method could be used? When I search openstack_dashboard directory, I find not a single file with both 'register' and 'Workflow' words [1]. So is it actually used by anyone? Maybe it should be just removed along with its incorrect sorting? Instead it could be replaced with functionality allowing to add Steps per instance.

[1] The search command was
grep -lr Workflow openstack_dashboard | xargs grep --color register

[Gabriel Hurley (gabriel-hurley)]

The reason for the incomplete sorting existing is so that third-party code can add or remove steps, which is relatively important. I'll give you that it seems to not be affecting anyone currently though. An improved sorting algorithm would be more than welcome.

[Kent Wang (k.wang)]

Looks like this fix was already released here: https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/data_processing/utils/workflow_helpers.py#L218