feat(pipelines): stack-level steps#16215
feat(pipelines): stack-level steps#16215mergify[bot] merged 8 commits intoaws:masterfrom kaizencc:pipelines
Conversation
|
|
rix0rrr
left a comment
There was a problem hiding this comment.
I like where this is going!
| for (const approval of props.changeSetApproval) { | ||
| for (const stack of approval.stacks) { | ||
| // getStackByName will throw if stack does not exist | ||
| const stackArtifact = assembly.getStackByName(stack.stackName); |
There was a problem hiding this comment.
I think I'd rather select the stack by selecting by construct path, that's guaranteed to be unique while stackName is only likely to be unique.
stack.node.path is the path, not sure which assembly.getStack() you need to be using there. It might not exist, in which case you have to iterate over all artifacts, check only for the stack artifacts and compare the nodePath (or hierarchicalId) attributes.
There was a problem hiding this comment.
am hoping that the change to assembly.getStackArtifact(stackstep.stack.artifactId) suffices.
packages/@aws-cdk/pipelines/lib/helpers-internal/pipeline-graph.ts
Outdated
Show resolved
Hide resolved
| stackGraph.add(prepareNode); | ||
| deployNode.dependOn(prepareNode); | ||
| firstDeployNode = prepareNode; | ||
| if (preNodeChain) { |
There was a problem hiding this comment.
I don't mind this tangle of ifs as a first pass to make it work, but I think there's more beautiful code in there that is more clear with less case analysis.
We want to express something like this:
if (changeSet && !prepare) {
throw error
}
// pre here
// prepare here
// changeset here
// deploy here
// post here
Feels that with a couple of properly-chosen helper functions (that deal transparently with undefineds or empty arrays) we could get the code to read a lot more straightforward.
WDYT?
There was a problem hiding this comment.
This may need more discussion. Agree that the ifs is ugly. With my latest attempt to remove the node chains, I've decided to utilize the existing addPrePost() and add my own addChangeSet() method. Now, the logic follows similarly to addWave and other areas where it looks more like this:
// prepare here
// deploy here
// pre/changeset/post here, if necessary
While I do think there is still opportunity to clean things up, you may have to elaborate more on the properly-chosen helper functions to achieve this.
| const postNode = this.addAndRecurse(p, parent); | ||
| postNode?.dependOn(...currentNodes.nodes); | ||
| } | ||
| return preNodes; |
There was a problem hiding this comment.
Not sure if modifying addPrePost() like this is acceptable. The alternative option is to reuse a lot of this code, since all I really want to do is return a NodeCollection to add stack-level dependencies on it later.
| stackGraph.add(deployNode); | ||
|
|
||
| // node or node collection that represents first point of contact in each stack | ||
| let firstDeployNode; |
There was a problem hiding this comment.
it feels a little wrong to continue calling this firstDeployNode given that it could be a node collection, but I'm not sure if it is that big of a deal.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Stack-level steps are available via the legacy API but not the modern API. This PR introduces the
stackStepsproperty toAddStageOpts, allowing the user to specify additional steps at the stack level rather than the stage level. You can specify a step using thepre,changeSetorpostoptions.presteps happen beforestack.prepare.changeSetsteps happen betweenstack.prepareandstack.deploy.poststeps happen afterstack.deploy.A primary use case is to add a
ManualApprovalStepinstackSteps.changeSetto verify changes to the stack before deployment.Closes #16148.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license