Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codepipeline: There should be a way to insert a stage before an existing one (and not append) #347

Closed
eladb opened this issue Jul 16, 2018 · 7 comments · Fixed by #460
Closed
Assignees

Comments

@eladb
Copy link
Contributor

eladb commented Jul 16, 2018

Currently, Pipelines only allows appending stages. This creates a limitation in usability. For example, we have a general purpose construct that defines a pipeline for code library builds. The current pipeline has two stages: build and release. We wish to implement a method .addTest(...), which will conditionally "inject" another stage between "build" and "release" only if it's called. If there are no tests, there should still be two stages.

Maybe we can do something like new Stage(pipeline, 'Bla', { insertBefore: someOtherStage }).

@skinny85
Copy link
Contributor

@eladb what do you think of something like this:

new Stage(pipeline, 'Stage', {
    atIndex: 3,
});

similarly, like you can do arr.splice(index, 0, item) in JavaScript? We would add a Pipeline#stagesCount() method to query how many Stages does a Pipeline currently have.

Thoughts on this?

@eladb
Copy link
Contributor Author

eladb commented Jul 26, 2018

I think atIndex is fine, but insertBefore would be more useful when you have another Stage object. We can easily support both (mutually exclusive).

@skinny85
Copy link
Contributor

Do you also want something like insertAfter: someOtherStage? Or should it only be insertBefore?

@skinny85
Copy link
Contributor

I started working on this, and there's an interesting conundrum.

Currently, the Stage is added to the Pipeline when it's added as a sub-construct, by overriding addChild in Pipeline. However, there is no way to pass any props there to insert it into a different position in the stages array - addChild takes a Construct and a name, that's it (and it's defined in Construct).

I implemented a temporary workaround of calling Pipeline#appendStage method in the Stage constructor, but that means making Pipeline#appendStage public, which is pretty bad...

Any ideas on how to get around this?

@skinny85
Copy link
Contributor

Gah. I tried to work around it by making props on the Stage public, and reading them in Pipeline#appendStage. But that doesn't work - that call is made in the constructor of Stage, in the call to super(parent, name) (the Construct constructor), and at that moment the fields of the Stage object are not yet set... :(

@skinny85
Copy link
Contributor

The only way I could come up with that didn't suck was changing the Construct API slightly. PR: #437

@skinny85
Copy link
Contributor

skinny85 commented Aug 1, 2018

Moved to PR #460 after problems with the fork when the repo was changed to public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants