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

(pipelines): generates warning since CDK 2.128.0 due to addition of v2 pipeline support in aws-codepipeline #29190

Closed
gossandr opened this issue Feb 21, 2024 · 2 comments · Fixed by #29199
Labels
@aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@gossandr
Copy link
Contributor

Describe the bug

pipelines is the opinionated construct library for creating codepipelines. while not labeled as such, i think it is a L3 Pattern Construct.

Since CDK 2.128.0, this construct generates a warning because PipelineType is not being provided when pipelines creates the aws-codepipeline.

This warning should have been caught before the release of 2.128.0, but instead the test that caught it was changed to effectively check nothing. See 40ffe2b#diff-8e15d23929b5ff6c128d73523da6a4322f975856fea30df4f969a21edf5f4eb2

I'm calling this an introduced bug because it breaks when running cdk synth --strict

Expected Behavior

I expect pipelines to not generate this warning by properly providing PipelineType when creating the underlying code pipeline resource.

Current Behavior

Currently it generates a warning like this: [Warning at example] V1 pipeline type is implicitly selected when pipelineType is not set. If you want to use V2 type, set PipelineType.V2. [ack: @aws-cdk/aws-codepipeline:unspecifiedPipelineType]

This warning will cause cdk synth strict to fail, which breaks numerous pipleines of ours based on the pipelines library.

Reproduction Steps

see example code from the docs

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { CodePipeline, CodePipelineSource, ShellStep } from 'aws-cdk-lib/pipelines';

export class MyPipelineStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const pipeline = new CodePipeline(this, 'Pipeline', {
      pipelineName: 'MyPipeline',
      synth: new ShellStep('Synth', {
        input: CodePipelineSource.gitHub('OWNER/REPO', 'main'),
        commands: ['npm ci', 'npm run build', 'npx cdk synth']
      })
    });
  }
}

Possible Solution

For example, in this code block: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts#L467-L480

change it to look something like this:

    } else {
      this._pipeline = new cp.Pipeline(this, 'Pipeline', {
        pipelineName: this.props.pipelineName,
        pipelineType: cp.PipelineType.V1,
        crossAccountKeys: this.props.crossAccountKeys ?? false,
        crossRegionReplicationBuckets: this.props.crossRegionReplicationBuckets,
        reuseCrossRegionSupportStacks: this.props.reuseCrossRegionSupportStacks,
        // This is necessary to make self-mutation work (deployments are guaranteed
        // to happen only after the builds of the latest pipeline definition).
        restartExecutionOnUpdate: true,
        role: this.props.role,
        enableKeyRotation: this.props.enableKeyRotation,
        artifactBucket: this.props.artifactBucket,
      });
    }

Additional Information/Context

No response

CDK CLI Version

2.128.0

Framework Version

No response

Node.js Version

18.2.0

OS

macos

Language

TypeScript

Language Version

No response

Other information

This was also mentioned in #29146 but this issue is looking to add v2 support. I haven't dug that deep to see how much other things would have to change to support that. This issue is merely looking to resolve the warning introduced. While this warning is suppressible, it isn't intuitive. Pipelines should at a minimum provide the PipelineType when it creates the pipeline to prevent this warning from being geneerated. v2 support as mentioned in the other issue would also resolve this, but I assume it takes longer than simply fixing this bug

@gossandr gossandr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 21, 2024
@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label Feb 21, 2024
@pahud
Copy link
Contributor

pahud commented Feb 21, 2024

Yes we should consider to remove the warning. Feel free to submit a PR if this is something you'd like to pick up with. Thank you.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 21, 2024
@mergify mergify bot closed this as completed in #29199 Feb 21, 2024
mergify bot pushed a commit that referenced this issue Feb 21, 2024
…f v2 pipeline support in aws-codepipeline (#29199)

### Issue # (if applicable)

Closes #29190 

### Reason for this change

CDK v2.128.0 introduced a warning in aws-codepipeline to warn users of the implicit behavior now that v2 pipelines are supported in CDK. This warning can cause established pipelines to fail if they are using cdk synth --strict. The warning can be suppressed, but the better fix is to have this module supply the `PipelineType` added in CDK v128.0 and set it to `v1`. A future change would have to address adding v2 pipeline support to this module, this only resolves the introduced warning. 

### Description of changes

- added the optional (and new) `PipelineType` property in the creation of the codepipeline in https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts#L467-L480
- slightly modified the README where it mentions using aws-codepipeline to include specifically an mention of v2 as a reason to use aws-codepipeline (until of course v2 is added properly in this lib)

### Description of how you validated changes

- reverted a change in 40ffe2b as I believe this previously caught the new warning and was changed to fix the test rather than looking into the new warning. This would catch the warning if `PipelineType` is not supplied

### Checklist
- [ x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

GavinZZ pushed a commit that referenced this issue Feb 22, 2024
…f v2 pipeline support in aws-codepipeline (#29199)

### Issue # (if applicable)

Closes #29190 

### Reason for this change

CDK v2.128.0 introduced a warning in aws-codepipeline to warn users of the implicit behavior now that v2 pipelines are supported in CDK. This warning can cause established pipelines to fail if they are using cdk synth --strict. The warning can be suppressed, but the better fix is to have this module supply the `PipelineType` added in CDK v128.0 and set it to `v1`. A future change would have to address adding v2 pipeline support to this module, this only resolves the introduced warning. 

### Description of changes

- added the optional (and new) `PipelineType` property in the creation of the codepipeline in https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts#L467-L480
- slightly modified the README where it mentions using aws-codepipeline to include specifically an mention of v2 as a reason to use aws-codepipeline (until of course v2 is added properly in this lib)

### Description of how you validated changes

- reverted a change in 40ffe2b as I believe this previously caught the new warning and was changed to fix the test rather than looking into the new warning. This would catch the warning if `PipelineType` is not supplied

### Checklist
- [ x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
2 participants