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: Missing props unused check for CodePipeline #20334

Closed
simonkarman opened this issue May 13, 2022 · 3 comments · Fixed by #20423
Closed

pipelines: Missing props unused check for CodePipeline #20334

simonkarman opened this issue May 13, 2022 · 3 comments · Fixed by #20423
Labels
@aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@simonkarman
Copy link

Describe the bug

In the constructor of pipelines.CodePipeline checks are done to verify that pipelineName and crossAccountKeys are not set if a pipeline is already provided through the props. However this check is missing for the reuseCrossRegionSupportStacks property.

Expected Behavior

Providing both the reuseCrossRegionSupportStacks parameter and the codePipeline parameter to a pipelines.CodePipeline should fail.

Current Behavior

However, right now: Providing both the reuseCrossRegionSupportStacks parameter and the codePipeline parameter to a pipelines.CodePipeline does not fail, although it should.

Reproduction Steps

The following should fail.

    import { aws_codepipeline as cp, pipelines } from 'aws-cdk-lib';
    new pipelines.CodePipeline(scope, 'CodePipeline', {
      reuseCrossRegionSupportStacks: true,
      codePipeline: new cp.Pipeline(scope, 'Pipeline'),
      synth: new pipelines.ShellStep('Synth', { commands: ['ls'] }),
    });

Possible Solution

Add the following piece of code to line 357 of the CodePipeline.

      if (this.props.reuseCrossRegionSupportStacks !== undefined) {
        throw new Error('Cannot set \'reuseCrossRegionSupportStacks\' if an existing CodePipeline is given using \'codePipeline\'');
      }

Additional Information/Context

No response

CDK CLI Version

2.16.0 (build 4c77925)

Framework Version

No response

Node.js Version

v14.18.0

OS

MacOS

Language

Typescript

Language Version

3.9.7

Other information

No response

@simonkarman simonkarman added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 13, 2022
@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label May 13, 2022
@NGL321 NGL321 added p2 and removed needs-triage This issue or PR still needs to be triaged. labels May 13, 2022
@NGL321 NGL321 added the effort/small Small work item – less than a day of effort label May 13, 2022
@NGL321
Copy link
Contributor

NGL321 commented May 13, 2022

Hey @simonkarman,

Thank you for reporting this! This seems like a simple fix, but it wont be high on the priority list (since even small fixes would require testing). Since you already have a good grasp on the solution, feel free to create a PR to resolve this following the Contribution guidelines and send it my way for review.
If you are unable to or don't have time, someone should address this eventually.

@simonkarman
Copy link
Author

Thanks @NGL321! And it seems like @tezz-io beat me to it, thanks for the help!

@mergify mergify bot closed this as completed in #20423 Jul 14, 2022
mergify bot pushed a commit that referenced this issue Jul 14, 2022
… existing pipeline is used (#20423)

----
Added prop check for reuseCrossRegionSupportStacks in CodePipeline which fixes #20334 

Added unit tests for all three prop checks.

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
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.

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/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants