-
Notifications
You must be signed in to change notification settings - Fork 37
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: waves for parallel builds with github options #490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @giseburt! Thanks for the PR, and sorry for the delay in getting back to you. I really appreciate your continued interest in this library! Can you do the following steps to help create an easier entrypoint into this PR:
- no stylistic changes -- it's cluttering the PR and makes it harder to find actual necessary changes. I'm not against it, but if you want to add them in they deserve to be in its own PR, with a way to enforce the same style in the future.
- a README update -- The PR title tells me that this will support waves, but nowhere in the tests do I see how that would work in practice. I would like to work backwards from the use case here, in that I'd like to see what a README entry for the new feature would look like, before I dive into the nitty gritty implementation.
Sorry @kaizencc, I thought I had all the style changes removed. I have a format-on-save set in VSCode. I also thought I had it set to use the styles int he repo, but I've missed something, so it's making bigger changes than just the aslant that projen does. I'll adjust. I also thought I had changed the ReadMe, but those are lost now, so I'll redo. |
Ok, @kaizencc, I updated the docs to include the new objects and how to use them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @giseburt! Some initial thoughts. I'm still mostly interested in seeing in visual form what a pipeline with waves looks like in GitHub Workflows.
Ok, @kaizencc, I believe I have made all the requested changes and answered all questions. I'll keep an eye out or any more you might have. Thank you! |
Ok, @kaizencc - I think I got all the changes you asked for. |
@kaizencc any update on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again @giseburt. Sorry for the delayed response. This is pretty much good to go and I'm prepared to approve tomorrow. I just have a few typo fixes and one comment about having both addWave
and addGitHubWave
as viable APIs, it might be confusing which one to use. Once that's addressed we're good!
Also, we recently added support for rosetta
in our cdklabs repos too. You'll need to revisit the README and make sure that the examples compile. It might help to look at what I did in #514 to get our current examples to compile.
public addWave(id: string, options?: WaveOptions): Wave { | ||
return this.addGitHubWave(id, options); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm we probably just want to direct users to use addGitHubWave
instead of addWave
entirely, right? It feels confusing to have both addWave
and addGitHubWave
as part of the API.
I think we should consider either throwing an error when you use addWave
and explaining that you should be using addGitHubWave
, or at the very least, providing a warning (via Annotations
construct) that you should be using addGitHubWave
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I spent a while trying to do this one to determine that it cannot be done, at least not without wrecking other things.
PipelineBase.addStage
calls addWave
internally. We'd have to rewrite addStage
as well. I don't think it's worth it.
I feel like the real solution would be upstream (adjust PipelineBase
, etc.), but I can't determine the best route at the moment.
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Ok, I once more believe I have all the changes you requested, or have a good reason not to (and commented to that elsewhere). I did have to make some adjustments for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!
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.