feat(appconfig): L2 constructs #26467
Conversation
github-actions
bot
added
the
beginning-contributor
There was a problem hiding this comment.
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.
chenjane-dev
changed the title
| }); | ||
| ``` | ||
|
|
||
| A hosted configuration with type: |
There was a problem hiding this comment.
This is a good place to educate users on what a type is. Same thing for validators and deployment strategy in the next two examples.
There was a problem hiding this comment.
Updated the README
| /** | ||
| * Defines the deployment strategy ID's of AWS AppConfig predefined strategies. | ||
| */ | ||
| export enum PredefinedDeploymentStrategyId { |
There was a problem hiding this comment.
Is it possible to keep this enum private? It seems like an implementation detail that should not be exposed.
There was a problem hiding this comment.
I kept this enum here, in case customers wanted to directly import these existing and AppConfig authored deployment strategies, instead of creating new ones through RolloutStrategy.ALL_AT_ONCE, etc. Not super necessary but it might add some flexibility for customers who want to directly use the existing authored deployment strategies, rather than creating new ones.
There was a problem hiding this comment.
I don't understand. What would they do with this enum other than create a RolloutStrategy?
There was a problem hiding this comment.
They could import it directly
DeploymentStrategy.fromDeploymentStrategyId(this, 'MyImportedDeploymentStrategy', PredefinedDeploymentStratgeyId.ALL_AT_ONCE);
which imports the existing AppConfig authored deployment strategies.
Opposed to
new DeploymentStrategy(this, 'MyDeploymentStrategy', {
rolloutStrategy: RolloutStrategy.ALL_AT_ONCE
});
which creates a new ALL_AT_ONCE deployment strategy (instead of using the existing AppConfig one)
| resourceName: this.applicationId, | ||
| }); | ||
|
|
||
| this.extensible = new ExtensibleBase(scope, this.applicationArn); |
There was a problem hiding this comment.
I'm sure there is a good reason to use composition instead of inheritance here, even if it creates a lot of duplication. You should document it somewhere.
There was a problem hiding this comment.
Where do you recommend documenting this?
There was a problem hiding this comment.
It depends on the reason, I guess. Is it because the constructs already extend cdk.Resource? If so, you could explain in the docstring of ExtensibleBase that it's supposed to be used with resources, but there is no way to inherit from two classes (at least within the jsii constraints).
Conversely, have you looked into making ExtensibleBase an abstract class that extends cdk.Resource?
There was a problem hiding this comment.
Will add that docstring. I don't think extending a cdk.Resource will suffice because IConfiguration is a Construct type and will have to inherit the ExtensibleBase still
otaviomacedo
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
mergify
bot
dismissed
otaviomacedo’s stale review
Pull request has been modified.
…ine (#26443) Expose `stateMachineRevisionId` as a readonly property to StateMachine whose value is a reference to the `StateMachineRevisionId` attribute of the underlying CloudFormation resource. Closes #26440 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…26455) Makes it less confusing ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…or of trigger failures (#26450) TriggerCustomResourceProvider takes only the status code of Invoke API call into account. https://github.com/aws/aws-cdk/blob/7a6f953fe5a4d7e0ba5833f06596b132c95e0709/packages/aws-cdk-lib/triggers/lib/lambda/index.ts#L69-L73 If invocationType is `EVENT`, Lambda function is invoked asynchronously. In that case, if Lambda function is invoked successfully, the trigger will success regardless of the result for the function execution. I add this consideration into README. Closes #26341 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
Add missing Aurora Engine Version 3_02_3. See Aurora docs at https://docs.aws.amazon.com/AmazonRDS/latest/AuroraMySQLReleaseNotes/AuroraMySQL.Updates.3023.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…o-op test (#26457) Test was a no-op due to the typo.
Policy names with slashes (`/`) are not allowed when bootstrapping. For example: ``` cdk bootstrap --custom-permissions-boundary aaa/bbb ``` Would fail: ``` Error: The permissions boundary name aaa/bbb does not match the IAM conventions. ``` This fix allows to specify paths in the policy name. Closes #26320. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…26297) Reported issue with `diff` is that it treats the fail return status for cases when there are actual diffs, making it hard to know what happened with pipeline automations. The proposed solution adds a logging statement with a similar format that is used for deploy (but here the total time is reported) specifying how many stacks have differences, as presented below. As a result, it will be possible to check in pipelines for this logging statement to correctly detect situation when there are no actual changes, when there are changes, and when there are failures, since on failures this statement will not be present: Case of no changes: ✨ Number of stacks with differences: 0 Case of changes in 5 stacks: ✨ Number of stacks with differences: 5 Closes #10417. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…26466) Ran into an issue where the construct trace was incorrect in larger projects, specifically where there are constructs that contain multiple constructs. To get the construct trace tree we first construct a new tree that only contains the path to the terminal construct (the one with the trace). We drop all the other constructs in the tree that don't relate. For example, if we had a tree like: ``` ->App -->MyStage --->MyStack1 ---->MyConstruct ----->Resource --->MyStack2 ---->MyConstruct ----->Resource --->MyStack3 ---->MyConstruct ----->Resource ``` And we want to get a new tree for `/App/MyStage/MyStack2/MyConstruct/Resource` it should look like this: ``` ->App -->MyStage --->MyStack2 ---->MyConstruct ----->Resource ``` We weren't correctly generating the new tree correctly and would always end up with the tree being the first item in the list. I've updated one of the tests, and also tested this on a more complex application. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…refix (#26324) Optionally specify a prefix for the staging stack name, which will be baked into the stackId and stackName. The default prefix is `StagingStack`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Implementing `IGrantable` for cases when it's needed to grant permissions to a `Service` instance.
For example:
```
declare const bucket: IBucket;
const service = new apprunner.Service(this, 'Service', {
source: apprunner.Source.fromEcrPublic({
imageConfiguration: { port: 8000 },
imageIdentifier: 'public.ecr.aws/aws-containers/hello-app-runner:latest',
}),
});
bucket.grantRead(service);
```
Closes #26089.
----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… allowed to be specified as subscription and dead-letter queue (#26110) To send message to SQS queues encrypted by KMS from SNS, we need to grant SNS service-principal access to the key by key policy. From this reason, we need to use customer managed key because we can't edit key policy for AWS managed key. However, CDK makes it easy to create such a non-functional subscription. To prevent CDK from making such a subscription, I added the validation which throw an error when SQS queue encrypted by AWS managed KMS key is specified as subscription or dead-letter queue. Closes #19796 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Fix issue with order of `-f` flag and file path in `rm` command for `pnpm` esbuild bundling step to remove `node_modules/.modules.yaml` from output dir. This is continuing to cause bundling step to fail for `pnpm` >= 8.4.0 with no external `node_modules` specified per issue #26478. Solved by moving the `-f` flag before file path in the `rm` command and updating relevant unit test. Please note that I haven't adjusted the `del` command for windows env as not sure if same issue occurs in that env. Exemption Request: No changes to integration test output of `aws-lambda-nodejs/test/integ.dependencies-pnpm.js` and don't feel this warrants a separate integration test. Closes #26478. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
There was a problem hiding this comment.
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.
aws-cdk-automation
added
the
pr/needs-cli-test-run
|
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
6 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
@chenjane-dev, @otaviomacedo Any plans to reopen this? |
Completes RCF #499
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license