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): RententionProperties key should be PascalCase #23570

Closed
Wurstnase opened this issue Jan 5, 2023 · 8 comments
Closed

(timestream): RententionProperties key should be PascalCase #23570

Wurstnase opened this issue Jan 5, 2023 · 8 comments
Labels
@aws-cdk/aws-timestream Related to the @aws-cdk/aws-timestream package bug This issue is a bug.

Comments

@Wurstnase
Copy link
Contributor

Wurstnase commented Jan 5, 2023

Describe the bug

In the cloudformation documentation all property keys are PascalCase. With 2.56.0 the RetentionProperties for timestream changed to camelCase:

         "RetentionProperties": {
-          "MagneticStoreRetentionPeriodInDays": "1",
-          "MemoryStoreRetentionPeriodInHours": "1"
+          "magneticStoreRetentionPeriodInDays": "1",
+          "memoryStoreRetentionPeriodInHours": "1"
         },

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-timestream-table.html

Expected Behavior

PascalCase for properties

Current Behavior

camelCase for properties

Reproduction Steps

class TimestreamStack(Stack):
    def __init__(
        self,
        scope: Construct,
        id_: str,
        *,
    ):
        super().__init__(scope, id_)

        cfn_database = timestream.CfnDatabase(
            self,
            "Database",
            database_name="database",
        )

        timestream.CfnTable(
            self,
            "Table",
            database_name=cfn_database.ref,
            table_name="input",
            retention_properties=timestream.CfnTable.RetentionPropertiesProperty(
                magnetic_store_retention_period_in_days="1",
                memory_store_retention_period_in_hours="1",
            ),
        )

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

Framework Version

>= 2.56.0

Node.js Version

OS

Language

Python

Language Version

No response

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 Jan 5, 2023
@github-actions github-actions bot added the @aws-cdk/aws-timestream Related to the @aws-cdk/aws-timestream package label Jan 5, 2023
@peterwoodworth
Copy link
Contributor

Thanks for reporting this, we actually reverted this intentionally because the behavior you see in 2.55.0 was actually a version of the previous behavior. See this issue for more info #23404. And this issue is tracking a way to prevent this from occurring again #21767

From here forward, please fill in these properties as if they are any type (because they are!)

@peterwoodworth peterwoodworth added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 6, 2023
@Wurstnase
Copy link
Contributor Author

Wurstnase commented Jan 6, 2023

Why does the behavior of the dataclass change as well?

This is the example from the official documentation:
https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_timestream/CfnTable.html

# The code below shows an example of how to instantiate this type.
# The values are placeholders you should change.
from aws_cdk import aws_timestream as timestream

retention_properties_property = timestream.CfnTable.RetentionPropertiesProperty(
    magnetic_store_retention_period_in_days="magneticStoreRetentionPeriodInDays",
    memory_store_retention_period_in_hours="memoryStoreRetentionPeriodInHours"
)

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 6, 2023
@peterwoodworth
Copy link
Contributor

You won't want to use the dataclass - the example up top shows that the properties in the CfnTable construct should be specified as any - not as a dataclass

@peterwoodworth peterwoodworth added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 6, 2023
@Wurstnase
Copy link
Contributor Author

Wurstnase commented Jan 6, 2023

Any in this context is dict[str, str]?

p1: Any = {}  # ok
p2: Any = timestream.CfnTable.RetentionPropertiesProperty()  # ok

p3: dict = {}  # ok
p4: dict = timestream.CfnTable.RetentionPropertiesProperty  # error

so somewhere seems to be an error?

@peterwoodworth
Copy link
Contributor

Defining it like this works for me on latest version:

        timestream.CfnTable(
            self,
            "Table",
            database_name="name",
            table_name="input",
            magnetic_store_write_properties=dict(
                EnableMagneticStoreWrites=True
            ),
            retention_properties=dict(
                MagneticStoreRetentionPeriodInDays="1",
                MemoryStoreRetentionPeriodInHours="1",
            ),
        )

@peterwoodworth peterwoodworth added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jan 6, 2023
@Wurstnase
Copy link
Contributor Author

Wurstnase commented Jan 6, 2023

Yes, this would work for me also.

However, it is not an error to define it with the given dataclass. I like cdk because it helps me to define everything with the right typings and it is harder to missuse. But not in this case.

e.g. with cdk v1 (maybe not compatible in this case) has a complete example with the dataclass in the properties. Why is there this dataclass, and it will produce something which will produce a so similar cloudformation template in the end?

The only issue is, that the producing template becomes camelCase instead of PascalCase. It would give type hints, e.g. properties must be string and not integer.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 7, 2023
@Wurstnase
Copy link
Contributor Author

Wurstnase commented Jan 7, 2023

I understand now the issue. With the new cloudformation spec came the newly generated dataclasses. With the patch they became camelCase.

It looks like it is a dup with #8996.

@github-actions
Copy link

github-actions bot commented Jan 7, 2023

⚠️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.

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.
Projects
None yet
Development

No branches or pull requests

3 participants