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

feat(codepipeline): handle non-CFN cross-region actions #3777

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Aug 24, 2019

Adds support for actions other than the CloudFormation ones to be cross-region.

It works similarly to the cross-account support: passing a resource from a different region into a CodePipeline action makes the action implicitly cross-region.

Closes #3387


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

@skinny85 skinny85 requested a review from eladb August 24, 2019 00:07
@skinny85 skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Aug 24, 2019
@skinny85
Copy link
Contributor Author

This should probably wait for #3694 to be merged before merging this, as it touches a similar area. But feel free to review right now (#3694 shouldn't have a huge impact on the code).


const actionResource = action.actionProperties.resource;
if (actionResource) {
const actionResourceStack = Stack.of(actionResource);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we validate that actionProperties.region is the same or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said before, I don't think that's a great idea. This could be passed by a concrete action class, which the customer has no control over, and I wouldn't be able to get rid of the validation error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved
@skinny85 skinny85 force-pushed the feature/cross-region-non-cfn-actions branch 2 times, most recently from 75ac631 to 7b3e80a Compare September 11, 2019 03:12
@skinny85
Copy link
Contributor Author

Included comments & rebased on top of 1.8.0.

@mergify
Copy link
Contributor

mergify bot commented Sep 13, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@aws aws deleted a comment from mergify bot Sep 13, 2019
@aws aws deleted a comment from mergify bot Sep 13, 2019
@aws aws deleted a comment from mergify bot Sep 13, 2019
@aws aws deleted a comment from mergify bot Sep 13, 2019
@skinny85 skinny85 force-pushed the feature/cross-region-non-cfn-actions branch from 7b3e80a to 806ba1e Compare September 17, 2019 21:58
@skinny85
Copy link
Contributor Author

Rebased on top of newest master.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Outdated Show resolved Hide resolved

const actionResource = action.actionProperties.resource;
if (actionResource) {
const actionResourceStack = Stack.of(actionResource);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -2,6 +2,25 @@ import kms = require('@aws-cdk/aws-kms');
import s3 = require('@aws-cdk/aws-s3');
import cdk = require('@aws-cdk/core');

export class CrossRegionSupportConstruct extends cdk.Construct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the "Construct" posfix: CrossRegionSupport

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That I can't do: that name is already taken (by the customer-facing interface that's returned from the Pipeline's class crossRegionSupport method). CrossRegionSupportConstruct is private to the codepipeline package.

@skinny85 skinny85 force-pushed the feature/cross-region-non-cfn-actions branch from 806ba1e to 21c0b7f Compare September 18, 2019 21:41
@skinny85 skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Sep 18, 2019
@skinny85
Copy link
Contributor Author

Rebased and included last comment.

@skinny85 skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Sep 18, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@skinny85 skinny85 merged commit b8b4c4d into aws:master Sep 18, 2019
@skinny85 skinny85 deleted the feature/cross-region-non-cfn-actions branch September 18, 2019 22:27
eladb pushed a commit that referenced this pull request Sep 23, 2019
Adds support for actions other than the CloudFormation ones to be cross-region.

It works similarly to the cross-account support: passing a resource from a different region into a CodePipeline action makes the action implicitly cross-region.

Closes #3387
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-region capabilities for non-CloudFormation CodePipeline Actions
4 participants