fix(athena): WorkGroup tags corruption #9085
Conversation
|
@iliapolo initial draft work here. A little more to do in my opinion, but looking for a little guidance from your side about what is necessary. Specifically the lint failure, which I think might be alleviated if I add a unit test here. |
|
@iliapolo - not sure if there is naming convention on these patches? But I've added a test to verify tags render correctly based on the docs: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-athena-workgroup.html#aws-resource-athena-workgroup--examples. I didn't write an integration test because that's not normally here for cfn constructs, but let me know. @udondan and @blimmer -- https://github.com/aws/aws-cdk/pull/9085/files#diff-337b645f01ad335010f3212222427af8R23-R32 does this look right to you? |
iliapolo
changed the title
iliapolo
changed the title
| "PropertyTypes": { | ||
| "AWS::Athena::WorkGroup.Tags": { | ||
| "patch": { | ||
| "description": "Sets AWS::Serverless::Function.S3Event.Events#UpdateType to Immutable", |
There was a problem hiding this comment.
Copy-paste leftovers.. :)
|
|
||
| test('No tests are specified for this package', () => { | ||
| expect(true).toBe(true); | ||
| test('test tags spec correction', () => { |
There was a problem hiding this comment.
Lets also add a test that configured Tags on CfnWorkGroup rather than using the Tag aspect.
| @@ -0,0 +1,36 @@ | |||
| { | |||
There was a problem hiding this comment.
I would add Workgroup to the file name.
|
@moofish32 P.S I also opened an issue (the very first one!) with the Athena resource provider in CloudFormation: aws-cloudformation/aws-cloudformation-resource-providers-athena#21 |
Is this repo/org the source of the CFN spec? I wasn't aware of this. Is there a list of als AWS orgs on GitHub? |
Im honestly not sure. I was poking around to see if there is a repo for the spec, and came across this specific athena resource provider. Lets wait a bit for their response and we can ask them. |
There was a problem hiding this comment.
@moofish32 I'd feel more comfortable if we also had a small integ test that makes sure this change actually deploys well. Would this be hard to accomplish?
I debated about this too. Should be possible, but I want to highlight that we won't see an Integ change when the spec updates, until we remove the patch. Since you won't see a change in the CFN output the test won't fire. So this is sort of a one time benefit and somebody needs to remember to kill this patch when the spec is fixed (hardest job in software). |
|
not quite enough time tonight to do the Integ test, but hopefully by Monday. |
|
@iliapolo - can you give this another pass now? |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from master 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 |
Since Athena does not have AWS constructs the tests are empty. What does the team think about me adding one test to verify this patch is correctly applied for the cfn generated constructs? Can I also get feedback on the file name choice I made or a pointer to conventions on the patch file names? Fixes aws#6936 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Since Athena does not have AWS constructs the tests are empty. What does
the team think about me adding one test to verify this patch is
correctly applied for the cfn generated constructs?
Can I also get feedback on the file name choice I made or a pointer to
conventions on the patch file names?
Fixes #6936
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license