-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(codepipeline): support environment variables for actions #34667
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
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.
change err msg change err msg change err msg
change integ
* | ||
* @default - no environment variables | ||
*/ | ||
readonly actionEnvironmentVariables?: EnvironmentVariable[]; |
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.
Avoided the name environmentVariables
because CodeBuildActionProps
that extends codepipeline.CommonAwsActionProps
and this CommonActionProps
has already used a property with the same name and an error occurs if the name is used in this props.
unused grantRead for secret unit tests
7eb8299
to
2a82dcd
Compare
// Don't use `grantRead` method for the secret because `secretsmanager:DescribeSecret` is not required. | ||
// See https://docs.aws.amazon.com/codepipeline/latest/userguide/action-reference-Commands.html#action-reference-Commands-envvars | ||
options.role.addToPrincipalPolicy(new iam.PolicyStatement({ | ||
actions: ['secretsmanager:GetSecretValue'], | ||
resources: [secretArn], | ||
})); |
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.
To use the SecretsManager, you must add the following permissions to your pipeline service role:
{
"Effect": "Allow",
"Action": [
"secretsmanager:GetSecretValue"
],
"Resource": [
"SECRET_ARN"
]
}
/** | ||
* Bind the environment variable to the action. | ||
* @internal | ||
*/ | ||
public abstract _bind(scope: Construct, actionProperties: ActionProperties, options: ActionBindOptions): void; |
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.
Originally, this abstract method was not made, but the concrete bind method was only created for SecretsManagerEnvironmentVariable
. However, considering that support for PARAMETER_STORE
may be added in the future, such as the one for CodeBuild, it is better to create the abstract method, so I did it this way. Otherwise, it would be necessary to implement the bind calling as follows. If more types are added in the future, the code will become even more complicated.
envVars?.forEach(envVar => {
if (envVar instanceof SecretsManagerEnvironmentVariable || envVar instanceof ParameterStoreEnvironmentVariable) {
envVar._bind(scope, this.actionProperties, options);
}
});
Now we can write like:
envVars?.forEach(envVar => {
envVar._bind(scope, this.actionProperties, options);
});
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
N/A
Reason for this change
CodePipeline supports the Environment Variables in actions, but L2 Pipeline with actions doesn't support it.
SECRETS_MANAGER
type is only supported for the Commands action.Description of changes
Added
EnvironmentVariable
withPlaintextEnvironmentVariable
andSecretsManagerEnvironmentVariable
.Describe any new or updated permissions being added
None.
Description of how you validated changes
Both unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license