-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(codepipeline): handle non-CFN cross-region actions #3777
Conversation
packages/@aws-cdk/aws-codepipeline/lib/full-action-descriptor.ts
Outdated
Show resolved
Hide resolved
|
||
const actionResource = action.actionProperties.resource; | ||
if (actionResource) { | ||
const actionResourceStack = Stack.of(actionResource); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
75ac631
to
7b3e80a
Compare
Included comments & rebased on top of |
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
|
7b3e80a
to
806ba1e
Compare
Rebased on top of newest |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
||
const actionResource = action.actionProperties.resource; | ||
if (actionResource) { | ||
const actionResourceStack = Stack.of(actionResource); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
806ba1e
to
21c0b7f
Compare
Rebased and included last comment. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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
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