-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(s3-tables): add L2 construct support for Table and Namespace resources #35023
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
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.
lgtm
| testCases: [defaultTableTest, schemaTableTest, importedTableTest], | ||
| }); | ||
|
|
||
| integ.node.addDependency(schemaTableTest); |
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.
Is there a reason we need this ?
I suppose the IntegTest stack would already depend on the creation of the 3 stacks : defaultTableTest, schemaTableTest, importedTableTest
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 was getting some resource not found errors for the API calls in the integration tests, IIRC adding this fixed that.
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.
Can you elaborate on this ? Were these errors on the assertions below or something else ? (side note : Assertions are deployed as another stack and are deployed post the IntegTest stack has been deployed)
Also SchemaTableStack creates table with a specific schema and compaction setting. Can we have an assertion check on that as well ?
Example : Assertions that check that the table with proper schema and compaction settings are created.
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.
Can you elaborate on this ? Were these errors on the assertions below or something else ? (side note : Assertions are deployed as another stack and are deployed post the IntegTest stack has been deployed)
Looks like this might be transient error, it works after re-running the integ tests. This gist has the full error message
Also SchemaTableStack creates table with a specific schema and compaction setting. Can we have an assertion check on that as well?
I've added assertions that validate the table maintenance configuration. It may not be feasible to add assertions for schema since we don't have an S3tables API that vends it.
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue
Related to #33054
Reason for this change
This adds L2 construct support for S3 Tables Namespace and Table resources
Description of changes
Namespace: defines an underlying CfnNamespace L1 ResourceTable: defines an underlying CfnTable L1 ResourceThese L2 constructs improve the user experience with strong type safety for input properties, more fail-fast validations, and the ability to import existing resources into CDK.
Example Usage
Describe any new or updated permissions being added
No permissions are being added with these changes.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license