python/aws_cdk.aws_secretsmanager: JSON.stringify in Python example

petertaylor3 commented 1 year ago

Describe the issue

Small thing, but a bit of JS seems to have snuck into this Python example.

Should probably be json.dumps rather than JSON.stringify.

# Templated secret with username and password fields templated_secret = secretsmanager.Secret(self, "TemplatedSecret", generate_secret_string=secretsmanager.SecretStringGenerator( secret_string_template=JSON.stringify({"username": "postgres"}), generate_string_key="password" ) )

Links

https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_secretsmanager/README.html

khushail commented 1 year ago

@petertaylor3 , thanks for pointing that out.We should update this in our docs.

stefanfreitag commented 1 year ago

@khushail @petertaylor3 If its fine for you both, I would like to look into this one.

btanand commented 1 year ago

@khushail @stefanfreitag I have fixed this issue and raised a PR (https://github.com/aws/aws-cdk/pull/25376 )

stefanfreitag commented 1 year ago

Hi @btanand! Thanks for looking into this and providing the pull request. I am not sure if the change from JSON.stringify(...) toJSON.dumps(...) is sufficient.

The example code in the README.md is Typescript (Javascript) and JSON supports only parse(...) and stringify(...) methods . Documentation for this is available here. My assumption is that your code change would break the Typescript example.
I would also assume that in Python you would need an import json and then it would be json.dumps(...) with all in lowercase.

By searching the code base I found also other places having the same "issue" of using JSON.stringify(...) in the Python examples, e.g. in aws_cdk.aws_events_targets.

My question would be on the translation process for the examples? Is that something happening in JSII and needs to be treated there?

peterwoodworth commented 1 year ago

Thanks for the discussion here, let's track this and related issues here. Thanks!

github-actions[bot] commented 1 year ago

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

corymhall commented 1 year ago

I'm going to keep this one open since we may be able to handle JSON specifically. We can add special cases for JSII to translate https://github.com/aws/jsii-rosetta/blob/8bb9a971e7f94f4a48d2f98eb3d7ba18420e48e6/src/languages/python.ts#L835

We would want to add translations for all of the languages though, not just python.

Dotnet has JsonSerializer.Serialize()
Go has json.Marshal
Java we probably need to invent some ConvertToJson function that we pretend to call

aws / aws-cdk