-
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): allow VDM settings at the configuration set level #30051
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.
Just minor issues, good work overall 👍
/** | ||
* The Virtual Deliverability Manager (VDM) options that apply to the configuration set | ||
* | ||
* @default - use account level settings |
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 should probably indicate what the default values are first, and mention that this can be overridden by account level settings
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.
Thank you for the review.
I've stated that "default" indicates the absence of settings at the Configuration level, and in such cases, settings at the account level will be applied.
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 should have made myself clearer. I think it's more important to tell the users that by default, VDM options are disabled, unless there are account level settings. Your current documentation only states that if they happen to have account level settings, those will be used. I would expect most of the CDK users will not be using account level settings.
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. Sorry for the lack of explanation.
However, my understanding is that the configuration set level settings need to be used together with the account level settings. (Deploying with only the configuration set level settings enabled is possible, but VDM itself cannot be used until the account level settings are enabled.)
That's why I tried to include the above as a Note in the README, but I would like your opinion.
I'm a little unsure about how it should be written...
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.
Oh, got it. The AWS documentation doesn't really make it explicit that they need to be enabled account wide first:
Virtual Deliverability Manager options are also provided at the configuration set level so you can define custom settings for how a configuration set will use engagement tracking and optimized shared delivery by overriding how they’ve been defined in Virtual Deliverability Manager.
I would really emphasize it in the TSDoc as well, the README seems to have it covered
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
Thank you. I have added a note to the README and docs that when setting Account Level settings with CDK, VdmAttributes should be used.
/** | ||
* Whether engagement metrics are enabled for the configuration set | ||
* | ||
* @default - use account level settings |
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.
See above
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's the same as the comment above.
/** | ||
* Whether optimized shared delivery is enabled for the configuration set | ||
* | ||
* @default - use account level settings |
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.
See above
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's the same as the comment above.
test('configuration set with engagement metrics only', () => { | ||
new ConfigurationSet(stack, 'ConfigurationSet', { | ||
vdmOptions: { | ||
engagementMetrics: true, | ||
}, | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::SES::ConfigurationSet', { | ||
VdmOptions: { | ||
DashboardOptions: { | ||
EngagementMetrics: 'ENABLED', | ||
}, | ||
GuardianOptions: Match.absent(), | ||
}, | ||
}); | ||
}); | ||
|
||
test('configuration set with optimized shared delivery only', () => { | ||
new ConfigurationSet(stack, 'ConfigurationSet', { | ||
vdmOptions: { | ||
optimizedSharedDelivery: true, | ||
}, | ||
}); | ||
|
||
Template.fromStack(stack).hasResourceProperties('AWS::SES::ConfigurationSet', { | ||
VdmOptions: { | ||
DashboardOptions: Match.absent(), | ||
GuardianOptions: { | ||
OptimizedSharedDelivery: 'ENABLED', | ||
}, | ||
}, | ||
}); | ||
}); |
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.
These tests seem a bit superfluous, given they're already being covered by the first one. It'd be more appropriate to test vdmOptions: {false, false}
and vdmOptions: {}
,
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've added test cases for when both options in vdmOptions are set to "DISABLED" and when vdmOptions itself is not configured.
Additionally, I'm sorry to say that I've realized from the results of these tests that the "DISABLED" setting cannot be implemented. Therefore, I'm revisiting the implementation of vdmOptions.
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.
Have you tried setting it to DISABLED
with ENABLED
account wide settings? That might be the only allowed use case
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.
Yes, I did and I could.
As I mentioned in a separate comment, the configuration set level settings can be deployed on their own, but to enable VDM, it is appropriate to configure them together with the account level settings.
Therefore, I also added the account level settings to the integration tests.
Co-authored-by: Jimmy Gaussen <[email protected]>
Co-authored-by: Jimmy Gaussen <[email protected]>
Co-authored-by: Jimmy Gaussen <[email protected]>
Co-authored-by: Jimmy Gaussen <[email protected]>
@nmussy |
new ses.VdmAttributes(this, 'VdmAccountLevelSettings', { | ||
engagementMetrics: true, | ||
optimizedSharedDelivery: true, | ||
}); |
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.
So there are the account-wide settings? I would indicate the construct name in the README and the TSDoc if that's the case
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.
As per the other comments, the README and docs have been updated.
new integ.IntegTest(app, 'ConfigurationSetVdmOptionsInteg', { | ||
testCases: [new TestStack(app, 'cdk-ses-configuration-set-vdmoptions-integ')], | ||
}); |
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's probably a good idea to run some assertions to make sure that the configuration sets and VdmAttributes are created with the expected values, see integ-tests. Let me know if you need some guidance
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
Thank you for your comments. I have added assertions using AwsApiCall for integration testing. Please let me know if there are any inappropriate parts.
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, thanks for the changes 👍
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.
Nice work, thanks for contributing!
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
### Issue # (if applicable) Closes aws#30041 . ### Reason for this change As described in the issue. ### Description of changes To allow VDM settings at the configuration set level, `vdmOptions` property has been added to the `ConfigurationSet` Construct. ```ts new ses.ConfigurationSet(this, 'ConfigurationSetWithVdmOptions', { vdmOptions: { // Add engagementMetrics: true, optimizedSharedDelivery: true, }, }); ``` ### Description of how you validated changes I implemented unit tests and integration tests for the three cases. 1. Configuration set with both engagement metrics and optimized shared delivery enabled. 2. Configuration set with only engagement metrics enabled and optimized shared delivery not configured. 3. Configuration set with only optimized shared delivery enabled and engagement metrics not configured. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
### Issue # (if applicable) Closes aws#30041 . ### Reason for this change As described in the issue. ### Description of changes To allow VDM settings at the configuration set level, `vdmOptions` property has been added to the `ConfigurationSet` Construct. ```ts new ses.ConfigurationSet(this, 'ConfigurationSetWithVdmOptions', { vdmOptions: { // Add engagementMetrics: true, optimizedSharedDelivery: true, }, }); ``` ### Description of how you validated changes I implemented unit tests and integration tests for the three cases. 1. Configuration set with both engagement metrics and optimized shared delivery enabled. 2. Configuration set with only engagement metrics enabled and optimized shared delivery not configured. 3. Configuration set with only optimized shared delivery enabled and engagement metrics not configured. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Closes #30041 .
Reason for this change
As described in the issue.
Description of changes
To allow VDM settings at the configuration set level,
vdmOptions
property has been added to theConfigurationSet
Construct.Description of how you validated changes
I implemented unit tests and integration tests for the three cases.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license