Skip to content
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

(timestream): retentionProperties as Json is not working anymore #23404

Closed
Wurstnase opened this issue Dec 20, 2022 · 4 comments · Fixed by #23425
Closed

(timestream): retentionProperties as Json is not working anymore #23404

Wurstnase opened this issue Dec 20, 2022 · 4 comments · Fixed by #23425
Assignees
Labels
@aws-cdk/aws-timestream Related to the @aws-cdk/aws-timestream package bug This issue is a bug. effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1

Comments

@Wurstnase
Copy link
Contributor

Describe the bug

With 2.55.0 the cfnspec changes.

3951f09#diff-0d394a72825e270916f40b00a99f6e46b17ad80d4a4c76d19dd1688883a0f96cR451
3951f09#diff-0d394a72825e270916f40b00a99f6e46b17ad80d4a4c76d19dd1688883a0f96cR457

      "MagneticStoreWriteProperties": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-timestream-table.html#cfn-timestream-table-magneticstorewriteproperties",
-         "PrimitiveType": "Json",
          "Required": false,
+         "Type": "MagneticStoreWriteProperties",
          "UpdateType": "Mutable"
        },
        "RetentionProperties": {
          "Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-timestream-table.html#cfn-timestream-table-retentionproperties",
-          "PrimitiveType": "Json",
          "Required": false,
+          "Type": "RetentionProperties",
          "UpdateType": "Mutable"
        },

At least with python, when you define the structure as a python-dict, the properties are missing in the cloudformation template.

        timestream.CfnTable(
            self,
            "Table",
            database_name=cfn_database.ref,
            table_name=TABLE_NAME,
-            retention_properties={
-                "MagneticStoreRetentionPeriodInDays": magnetic_store_retention_period_in_days,
-                "MemoryStoreRetentionPeriodInHours": memory_store_retention_period_in_hours,
+            retention_properties=timestream.CfnTable.RetentionPropertiesProperty(
+                magnetic_store_retention_period_in_days=str(magnetic_store_retention_period_in_days),
+                memory_store_retention_period_in_hours=str(memory_store_retention_period_in_hours),
            ),
        )

Expected Behavior

Defining the properties as a python-dict should synth the properties into the cloudformation stack.

Current Behavior

When defining the properties as a dict, the cloudformation stack has not properties.

Reproduction Steps

see python example above.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.55.1

Framework Version

No response

Node.js Version

OS

Language

Python

Language Version

3.9.16

Other information

No response

@Wurstnase Wurstnase added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 20, 2022
@github-actions github-actions bot added the @aws-cdk/aws-timestream Related to the @aws-cdk/aws-timestream package label Dec 20, 2022
@peterwoodworth
Copy link
Contributor

I'm sorry this regression occurred @Wurstnase, we try to avoid merging spec changes which will cause regressions for customers. Thank you for letting us know this happened 🙂

@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 20, 2022
@Wurstnase
Copy link
Contributor Author

Thanks for your quick response. Fortunately, we have snapshot tests for our stacks.

The most annoying thing about this error is that it is silent. Not an error. It just disappears. Maybe there is an idea that this could lead to an exception?

@mergify mergify bot closed this as completed in #23425 Dec 21, 2022
mergify bot pushed a commit that referenced this issue Dec 21, 2022
… not working as Json (#23425)

Fixes #23404 


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@madeline-k
Copy link
Contributor

Hey @Wurstnase, this has been fixed and released in version 2.56.0! We are keeping track of occurrences like this in this issue: #21767, and solutions/suggestions for this problem should go there. I like your suggestion of having an error message. It might be tricky to do since this manifests slightly differently in each target language.

brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Jan 20, 2023
… not working as Json (aws#23425)

Fixes aws#23404 


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Feb 22, 2023
… not working as Json (aws#23425)

Fixes aws#23404 


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-timestream Related to the @aws-cdk/aws-timestream package bug This issue is a bug. effort/small Small work item – less than a day of effort needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1
Projects
None yet
4 participants