feat(pipelines): refactor source stage and artifact handling #11401
Conversation
|
|
|
@rix0rrr as discussed the PR draft for the source action changes What I did:
Feedback is much appreciated |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
The moment this is supported in CFN/CDK I would set this as default in the CodeCommitSource for CDK Pipelines. This si what developers expect when accessing the source artifact. |
|
|
||
| /** | ||
| * The CodePipeline action build and synthesis step of the CDK app | ||
| * | ||
| * @default - Required unless `codePipeline` or `sourceAction` is given | ||
| */ | ||
| readonly synthAction?: codepipeline.IAction; | ||
| readonly synthAction?: ISynthAction; |
There was a problem hiding this comment.
I think it would be okay to call this synth: ISynth as well.
There was a problem hiding this comment.
Sure, will handle this when we discussed the rest.
| @@ -21,19 +23,28 @@ export interface CdkPipelineProps { | |||
| * | |||
| * @default - Required unless `codePipeline` is given | |||
| */ | |||
| readonly sourceAction?: codepipeline.IAction; | |||
There was a problem hiding this comment.
I'm SUPER debating whether it's worth to hard-break backwards compatibility, or whether we should be doing our best to maintain it in order to give people a reasonable migration path off of the current API.
I think I'd prefer the deprecation/migration path, but on the other hand I'm not sure exactly what that should or would look like, and I think the implementation will become too complicated.
Maybe we stage a grander refactoring of CDK Pipelines on a separate, longer-lived branch, until we're ready to release it?
There was a problem hiding this comment.
I was thinking of a backward-compatible way but I think this will make the code unreadable. The reason for dev preview is to experiment and break APIs when needed imho.
| */ | ||
| readonly cloudAssemblyArtifact: codepipeline.Artifact; | ||
| readonly sourceArtifact?: codepipeline.Artifact; |
There was a problem hiding this comment.
The goal is to auto-derive these artifacts. So we definitely shouldn't be adding them. I also think we don't necessarily want to give users the power to override them here.
There was a problem hiding this comment.
We need to if we want users to be able to provide a custom CodePipeline with existing source stage.
| // | ||
| } | ||
|
|
||
| public provideSourceAction(sourceArtifact: codepipeline.Artifact): codepipeline.IAction { |
There was a problem hiding this comment.
I think instead of provideSourceAction I'd be in favor of a more general addToPipeline(pipelineSourceProps): void; and instead of returning a single Action the ISource is free to do WHATEVER to mutate the pipeline. It can add a single Action, it can add twenty.
Feels like it gives more flexibility.
There was a problem hiding this comment.
Honestly, I am not sure if flexibility is what I want to achieve here. I want the user to provide a source not to do arbitrary stuff with the pipeline.
| * @param sourceArtifact The source artifact of the CodePipeline | ||
| * @param cloudAssemblyArtifact The artifact where the CloudAssembly should be emitted | ||
| */ | ||
| configureArtifacts(sourceArtifact: codepipeline.Artifact, cloudAssemblyArtifact: codepipeline.Artifact): void; |
There was a problem hiding this comment.
Feels like this could also have been an addToPipeline() method of some sort.
There was a problem hiding this comment.
I am a big fan of concrete APIs that exactly tell the user what to do, but we can switch to a more generic approach.
| /** | ||
| * The BitBucket repo slug. (e.g. "aws/aws-cdk") | ||
| */ | ||
| readonly slug: string; |
There was a problem hiding this comment.
This is not really a user-friendly term. Why not just repo ?
There was a problem hiding this comment.
I am not attached to the name. I just used the working from BB/GH
| * @example 'arn:aws:codestar-connections:us-east-1:123456789012:connection/12345678-abcd-12ab-34cdef5678gh' | ||
| * @see https://docs.aws.amazon.com/codepipeline/latest/userguide/connections-create.html | ||
| */ | ||
| readonly connectionArn: string; |
There was a problem hiding this comment.
Wow, is this required? That looks pretty bad :/
There was a problem hiding this comment.
That is the way BB is currently configured and GH will move to this too with GithubSourcev2
| /** | ||
| * The GitHub repo slug. (e.g. "aws/aws-cdk") | ||
| */ | ||
| readonly slug: string; |
| * | ||
| * @see https://docs.aws.amazon.com/codepipeline/latest/userguide/GitHub-create-personal-token-CLI.html | ||
| * | ||
| * @default - use a SecretsManager secret called GitHub |
There was a problem hiding this comment.
Ooo. On the one hand I like this level of defaulting, but I wonder if it's too much?
It'll be easy to forget and the error will be completely non-descriptive (I believe it'll be something like "Internal Error").
There was a problem hiding this comment.
Maybe there is a way to check if it exists?
| * CodePipeline action to synthesize CDK applications. | ||
| * The source artifact and the cloud assembly artifact are lazily initialized by the CDK pipeline. | ||
| */ | ||
| export interface ISynthAction extends codepipeline.IAction { |
There was a problem hiding this comment.
I don't think this needs to extend an IAction anymore.
There was a problem hiding this comment.
Good catch. It definitely should not be an IAction
|
I think it's worth me investing some effort into making the underlying Will start on that right now. |
What exactly do you want to make easier by this? Do you want to get rid of the factory style? |
|
Superseded by #12326 |
refs #10872
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license