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

feat: support waves and add better if control #377

Closed
wants to merge 16 commits into from

Conversation

giseburt
Copy link
Contributor

Fixes #376

Continuing the description there:

I've added GitHubStage which extends Stage, adding the properties of AddGitHubStageOptions to StageProps. Stage ignores them, and they quickly get lost in the conversion to a StageDeployment, so we also need to catch that conversion.

I made a subclass of Wave called GitHubWave since n both wave.addStage() and pipeline.addStage() (which is defined in pipeline_base as this.addWave(stage.stageName).addStage(stage, options)) the the Stage object itself is tossed.

GitHubWave takes the parent GitHubWorkflow as an additional parameter, and GitHubWorkflow.addWave() calls new GitHubWave(id, this, options). GitHubWave only overrides addStage() to call parent.addingStageFromWave() with the unaltered Stage in addition to super.addStage(stage, options).

I have also added GitHubWave.addStageWithGitHubOptions() to match GitHubWorkflow.addStageWithGitHubOptions() as well as GitHubWorkflow.addGitHubWave(), but those can likely go away.

The trick is the JSII rules preventing "overriding or implementing a member using covariant parameter or return types" - which means we cannot make addStage() or addWave() take different parameters or have a different return type. However, we can accept and return classes that are descendants of those types, and use instanceof to catch them and get the additional information and functionality that they carry. So, pipeline.addStage() can accept a GitHubStage as a Stage, and pipeline.addWave() can return a Wave that's actually a GitHubWave. This allows us to maintain the same call structure but pass more information when we want.

How well that works in the other languages is TBD. If the JSII rules are sufficient, then it should work. In the least, JSII compiles it. 😄

@kaizencc
Copy link
Contributor

This seems like an opinionated PR -- I'll review it when I have time, but it won't be this week.

@giseburt
Copy link
Contributor Author

I appreciate it.

I'm very much open to discussion on this.

I was partially aiming at enabling functionality and syntax found in the codebuild pipeline without having to use the escape hatches.

@giseburt
Copy link
Contributor Author

Hello @kaizencc,

I'd like to utilize these features (more importantly the Wave support), and would like to provide as much support as possible moving forward with this PR.

I'm open to discussion on how to move forward with this PR, and alter it as needed. It appears you may prefer to remove the if adjustments, perhaps moving them to another ticket?

Thank you!

@giseburt
Copy link
Contributor Author

Superseded by #490

@giseburt giseburt closed this Jan 20, 2023
mergify bot pushed a commit that referenced this pull request Mar 2, 2023
Fixes #489

Removed the `if`-specific code from #377 and rebased on top of the latest main.

With this PR it's possible to have Waves without losing GitHub-specific settings properties.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better flow control: Ability to use Waves, set if on individual stages/steps
2 participants