-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(ses): add configurationSetArn property on configurationSet resource #29903
feat(ses): add configurationSetArn property on configurationSet resource #29903
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request
|
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.
While I agree with your exemption request about the README and integ file changes, I think we need to cover the new property with a unit test
this.configurationSetArn = this.stack.formatArn({ | ||
service: 'ses', | ||
resource: 'configuration-set', | ||
resourceName: this.configurationSetName, | ||
}); |
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 think a getter method would be cleaner here, and would avoid having to duplicate this code in the Import
class:
public get configurationSetArn(): string {
return this.stack.formatArn({
service: 'ses',
resource: 'configuration-set',
resourceName: this.configurationSetName,
});
}
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.
@nmussy thanks for your comment. Not sure about your proposal, do you want me to implement a base class ConfigurationSetBase
that both Import
and ConfigurationSet
will extend in order for them to both have this method? Found references to this pattern in https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#abstract-base
I'll implement something based on that. Let me know if you had something else in mind :)
I was originally inspired by EmailIdentity
construct from aws-ses
which has this duplicated code.
aws-cdk/packages/aws-cdk-lib/aws-ses/lib/email-identity.ts
Lines 392 to 396 in 31492c6
public readonly emailIdentityArn = this.stack.formatArn({ | |
service: 'ses', | |
resource: 'identity', | |
resourceName: this.emailIdentityName, | |
}); |
aws-cdk/packages/aws-cdk-lib/aws-ses/lib/email-identity.ts
Lines 500 to 504 in 31492c6
this.emailIdentityArn = this.stack.formatArn({ | |
service: 'ses', | |
resource: 'identity', | |
resourceName: this.emailIdentityName, | |
}); |
6db21d0
to
7e81307
Compare
@nmussy thanks for your review. I deduplicated the code and added unit tests. Thanks :) ! |
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.
LGTM, just unsure about the @attribute
tag 👍
* @attribute | ||
*/ | ||
readonly configurationSetArn: 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.
The @attribute
might not be appropriate since it's derived from the resource ref, a maintainer should weigh in on this
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.
In case it helps, the exemple construct from the coding guideline uses @attribute
for the ARN property even if it is computed manually (like in this case) :
exempleResourceArn
is tagged with@attribute
aws-cdk/packages/@aws-cdk/example-construct-library/lib/example-resource.ts
Lines 70 to 83 in 6fdc458
/** | |
* The ARN of example resource. | |
* Equivalent to doing `{ 'Fn::GetAtt': ['LogicalId', 'Arn' ]}` | |
* in CloudFormation if the underlying CloudFormation resource | |
* surfaces the ARN as a return value - | |
* if not, we usually construct the ARN "by hand" in the construct, | |
* using the Fn::Join function. | |
* | |
* It needs to be annotated with '@attribute' if the underlying CloudFormation resource | |
* surfaces the ARN as a return value. | |
* | |
* @attribute | |
*/ | |
readonly exampleResourceArn: string; |
exempleResourceArn
is constructed manually infrom
static methods and constructor
aws-cdk/packages/@aws-cdk/example-construct-library/lib/example-resource.ts
Lines 362 to 368 in 6fdc458
public readonly exampleResourceName = exampleResourceName; // Since we have the name, we have to generate the ARN, // using the Stack.formatArn helper method from the core library. // We have to know the ARN components of ExampleResource in a few places, so, // to avoid duplication, extract that into a module-private function public readonly exampleResourceArn = core.Stack.of(scope) .formatArn(exampleResourceArnComponents(exampleResourceName));
The only doubt I have is regarding this precision : It needs to be annotated with '@attribute' if the underlying CloudFormation resource surfaces the ARN as a return value.
CloudFormation does not surface AWS::SES::ConfigurationSet
ARN.
Any chance a maintainer can review and weigh in on the decision to keep or remove the |
They'll address it when they review the PR 👍 |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
@nmussy any chance this PR can be reviewed before end of the week? It will apparently automatically be closed otherwise (see #29903 (comment)). |
You need to file an exemption request comment to address the issues, see this comment. And I can't speak to review delays, I'm not a part of the CDK team, just a community member 👍 |
7e81307
to
205cce7
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
The pull request linter fails with the following errors:
PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing ✅ A exemption request has been requested. Please wait for a maintainer's review. |
Issue
Closes #29902
Reason for this change
Adds
configurationSetArn
property onConfigurationSet
construct to ease up building Policy referencing such constructDescription of changes
Similar implementation to recently added
emailIdentityArn
property onEmailIdentity
construct. The implementation leverages the use ofstack.formatArn
to buildAWS::SES::ConfigurationSet
arnDescription of how you validated changes
Did not add any test as this would basically mean testing
formatArn
function itself.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license