-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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-actions): add pipeline invoke action support. #34039
base: main
Are you sure you want to change the base?
Conversation
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #34039 +/- ##
=======================================
Coverage 83.98% 83.98%
=======================================
Files 120 120
Lines 6976 6976
Branches 1178 1178
=======================================
Hits 5859 5859
Misses 1005 1005
Partials 112 112
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5981f91
to
66dd46d
Compare
@@ -0,0 +1,88 @@ | |||
import { Stack } from 'aws-cdk-lib'; | |||
import { RevisionType } from 'aws-cdk-lib/aws-codepipeline-actions'; | |||
import { app } from '../../aws-appmesh/test/integ.mesh-port-match'; |
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.
../../aws-appmesh/test/integ.mesh-port-match
Is this a mistake?
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.
revised.
const repo = new codecommit.Repository(stack, 'MyRepo', { | ||
repositoryName: 'my-repo', | ||
}); |
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.
CodeCommit has already stopped accepting new ones, so it would be better to use other source actions in case another developer takes over this test in the future, what do you think?
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.
Will update in the next revision. Thanks for mentioning
|
||
const integrationTest = new IntegTest(app, 'codepipeline-integ-test', { | ||
testCases: [stack], | ||
stackUpdateWorkflow: false, |
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.
stackUpdateWorkflow: false,
Is this necessary?
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.
Assertion is recommended actually. Not necessary.
const awsApiCall1 = integrationTest.assertions.awsApiCall('CodePipeline', 'getPipeline', { name: pipelineName }); | ||
awsApiCall1.assertAtPath('pipeline.name', ExpectedResult.stringLikeRegexp(pipelineName)); | ||
awsApiCall1.assertAtPath('pipeline.stages.1.actions.1.name', ExpectedResult.stringLikeRegexp('Invoke')); |
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.
How about using the assertions that actually run the pipeline (for startPipelineExecution
and getPipelineState
with Succeeded
status), which I thought could be used to check IAM permissions etc.
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.
I prefer the cdk integ test should be testing cdk input -> cfn input -> service
The goal of these tests are checking if backend service correctly received the parameters from cdk app. The IAM
and PipelineExecution
are more about functionalities, which have been well checked in our internal tests, so I don't think they're necessary..
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.
I understand your perspective, but I see it a bit differently. This is L2 construct, not L1 construct. L2 constructs are not just simple wrappers around CloudFormation - they provide sensible defaults and recommended configurations. That's why the bound
method automatically configures and sets appropriate IAM permissions by default.
However, there's always a possibility that CDK contributors might implement inappropriate IAM settings. Where would we catch such issues? Unit tests only verify input/output matching and can't cover this aspect. In my view, integ tests are precisely the place to verify the actual behavior of CDK's internal code.
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.
I'm not sure, as I haven't used this new action yet, but can this action be used cross-accounting?
If so, should we support it in this action class? Or is this action intended to be performed within a single account?
/** | ||
* The name of the pipeline that will, upon running, start the current target pipeline. | ||
* You must have already created the invoking pipeline. | ||
*/ | ||
readonly pipelineName: string; |
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.
How about using IPipeline
?
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.
Sorry, I was mistaken. I thought this was the name of the target pipeline, but it is the name of the pipeline that executes this action.
So it was my understanding that it is impossible to pass the pipeline to which this action belongs...
PS.) No, was my perception correct...?
Please let me know if my understanding is different. (It's a bit confusing, so even if we don't use IPipeline
, we might as well change the property name in the CDK.)
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.
I would insist on the pipelineName since the targetPipeline could not be created by cdk... If customers have a pipeline created by console or terraform, they cannot use IPipeline
here
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.
I'll revise here to reduce the ambiguity
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.
btw, https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-PipelineInvoke.html this doc is kind of confusing, I already asked the doc team to improve
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.
I would insist on the pipelineName since the targetPipeline could not be created by cdk... If customers have a pipeline created by console or terraform, they cannot use IPipeline here
The interfaces starting with 'I' like IPipeline
are specifically designed to import resources (called unowned resources) created outside of CDK (such as through the console or Terraform) into your stack.
You can use methods like fromPipelineArn
to virtually incorporate these external resources into your stack. This approach allows you to use convenient features like onEvent
methods on imported resources, unlike with primitive types.
const unownedPipelineArn = 'MyUnownedPipelineArn';
// importedPipeline: IPipeline
const importedPipeline = Pipeline.fromPipelineArn(this, 'ImportedPipeline', unownedPipelineArn);
// can use like construct
imported.onEvent('test');
console.log(imported.pipelineArn);
console.log(imported.pipelineName);
new OtherResource(this, 'OtherResource', {
pipeline: importedPipeline,
});
Additionally, using primitive type properties (i.e. physical attributes) in props is generally not recommended in CDK design.
https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#types
actions: ['codepipeline:StartPipelineExecution'], | ||
resources: ['*'], |
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.
This is related to the comment above, could IPipeline
in this props be used to properly narrow down the resources instead of *
? (using pipeline.pipelineArn
)
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.
This may be relevant to the following comment...
I'm not sure, as I haven't used this new action yet, but can this action be used cross-accounting?
If so, should we support it in this action class? Or is this action intended to be performed within a single account?
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.
related: #34039 (comment)
} | ||
/** |
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.
} | |
/** | |
} | |
/** |
readonly revisionType: RevisionType; | ||
/** |
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.
readonly revisionType: RevisionType; | |
/** | |
readonly revisionType: RevisionType; | |
/** |
S3_OBJECT_VERSION_ID = 'S3_OBJECT_VERSION_ID', | ||
|
||
/** |
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.
S3_OBJECT_VERSION_ID = 'S3_OBJECT_VERSION_ID', | |
/** | |
S3_OBJECT_VERSION_ID = 'S3_OBJECT_VERSION_ID', | |
/** |
/** | ||
* A pipeline-level variable used for a pipeline execution. | ||
*/ | ||
export interface Variable { | ||
/** | ||
* The name of a pipeline-level variable. | ||
*/ | ||
readonly name: string; | ||
|
||
/** | ||
* The value of a pipeline-level variable. | ||
*/ | ||
readonly value: string; |
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.
If the variables generated here are to be used in the target pipeline, do they need to be called in the following form?: #{variables.xxx}
If so, wouldn't it be more convenient to create a class with a method to wrap this behaviour and use an instance of that class?
The class in that file is for VariableDeclaration
so we can't use it here, and we need a new class.
P.S.) "Variable namespace" is available in the management console, but cannot it be specified in CloudFormation?
@@ -0,0 +1,88 @@ | |||
import { Stack } from 'aws-cdk-lib'; | |||
import { RevisionType } from 'aws-cdk-lib/aws-codepipeline-actions'; |
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.
We can use the definition below this one. (line 4)
import * as cpactions from 'aws-cdk-lib/aws-codepipeline-actions';
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.
nit
*/ | ||
COMMIT_ID = 'COMMIT_ID', | ||
/** | ||
* The revision type is a image digest. |
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.
* The revision type is a image digest. | |
* The revision type is an image digest. |
*/ | ||
IMAGE_DIGEST = 'IMAGE_DIGEST', | ||
/** | ||
* The revision type is a s3 object version id. |
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.
* The revision type is a s3 object version id. | |
* The revision type is an S3 object version id. |
S3_OBJECT_VERSION_ID = 'S3_OBJECT_VERSION_ID', | ||
|
||
/** | ||
* The revision type is a s3 object version key. |
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.
* The revision type is a s3 object version key. | |
* The revision type is an S3 object version key. |
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.
Thanks for bring this up lol. I'll talk to our AI team for this issue
import * as cpactions from '../../lib'; | ||
import { RevisionType } from '../../lib'; |
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.
Could be merged.
|
||
/* eslint-disable quote-props */ | ||
|
||
describe('', () => { |
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.
Would be removed.
})); | ||
}); | ||
|
||
function stackIncludingPipelineInvokeCodePipeline(pipelineName: string, app?: App) { |
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.
It would be good to get it out of the 'describe' so that it can be used in other tests.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # 33818
Closes #33818
#33818
Reason for this change
Codepipeline team launched pipeline invoke action last year, but not available in cdk library yet.
Description of changes
Pipeline invoke action support
Describe any new or updated permissions being added
no
Description of how you validated changes
Unit test, local deployment, integ test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license