New graph visitor

[timbrown5]

Update PipelineStepVisitor to (almost) replace PipelineNodeGraphVisitor.
I think the implementation is easier to understand - nodes are parsed when we hit a start block, not when we child the child start block - I found getting parent often returned null (which might be the cause of some of the issues).
@mikedld is this sort of what you were thinking of in: #231 (comment) ?

Known Issues:

• PipelineStepVisitor gets called twice in PipelineStepsApi - once for Stages and once for steps. Could we store a Visitor for a session (or would that make it harder to update)?
• New implementation adds additional nodes to the graph (Parallel Branches). This breaks some tests (but doesn't seem to affect
• the new PipelineStepVisitor incorrectly sets the stage if parallel blocks to not_built. I think this is the cause of some of the unit test failures. In truth setting the correct state of the NodeNodeWrapper is surprisingly tricky.
• Not sure if this is fixed completely, but changing the order of status checks in 'handleChunkDone' has resolved some of the issues I was seeing.

Seems to fix #216 (added test for this):

But not #51 (could be some updates require on the frontend?):

Pipeline:

node {
    parallel([
        "A": {
            stage("Build") {
                echo("Build A")
            }
            stage("Test") {
                parallel([
                    "A1" : {
                       echo("Test A1")
                    },
                    "A2" : {
                       echo("Test A2")
                    }
                ])
            }
        },
        "B": {
            stage("Build") {
                echo("Build B")
            }
            parallel([
                "B1" : {
                   echo("Test B1")
                },
                "B2" : {
                   echo("Test B2")
                }
            ])
        }
    ])
}

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[stuartrowe]

@timbrown5 thanks! This seems to fix #216 as well.

[timbrown5]

@stuartrowe thanks for that 👍. I guess I should add a test for that to make sure it doesn't get broken in the future.
My belief if that this gets an accurate representation of the node graph. The issue is that the other classes rely on the behaviour of the PipelineNodeGraphVisitor, which seems to have grown organically and so creates a slightly esoteric representation. Parallel branches are a good example of this:

In the graph there is a specific node for this, but the PipelineNideGraphVisitor ignored this and either:

• Added blocks to the parent stage (if it exists).
• (If the parallel block isn't wrapped in a stage) add a synthetic stage to wrap the parallel block in.

This seems needless as you can just return the parallel branches wrapped in the parallel block.
Unfortunately, the new representation means updating some of the other classes, in the example above to collapse parallel block nodes when.there is a parent stage.
I started making the PipelineStepVisitor do this and was thinking of adding feature flags to enable certain aspects, but now I wonder if I should create a wrapper/facade to covert the new representation into the old.

[timbrown5]

With the latest change, the graph looks better with a Nested parallel blocks (the stages aren't missing), but still not correct (they aren't branched, but are children):

[timbrown5]

FYI, this doesn't fix may not the Pipeline Graph for nested Pipelines:

For reference, here are the graphs from master:

Update:
On second viewing, maybe the new Graphs are better than the old ones - just not what I expected.

[timja]

Suggested change

	// We only want to do this once, so return early if we jhave an existing value.
	// We only want to do this once, so return early if we have an existing value.

[timbrown5]

I'm leaving this as a draft as I don't know what additional checks would be required to land.

I'm, also a little torn with changing (breaking?) the Node Graph - I could revert it to still use PipelineNodeGraphVisitor (and move the changes to PipelineGraphApi to a new class - probably PipelineTreeApi.
Then the console via and graph via can call different Visitors - and the GraphView moved at a later point if/when the frontend code is updated.

[timja]

no real opinion at this point, whatever you think is best, test coverage gives confidence in these areas.

[timbrown5]

I think the tests check that the nodes are structured correctly but not that they are the expected type.
The types are probably different which might be the cause of why the old and new graphs look different.

[timbrown5]

I think fixing the Graph front-end for nested parallel stages can be tracked in #51.
I have added an example Pipeline and how I think it should be presented (although I could be wrong)
#51 (comment)