-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
RDS: DatabaseProxy dbProxyName is not autogenerated by CloudFormation #18578
RDS: DatabaseProxy dbProxyName is not autogenerated by CloudFormation #18578
Comments
Hey @AustinGomez, thanks for opening the issue. Indeed, this is a mistake in the documentation, and we should fix that. Using the Would you consider opening us a Pull Request, improving the docs? The offending line is here. Here's our "Contributing" guide: https:/aws/aws-cdk/blob/master/CONTRIBUTING.md. Thanks, |
Hi @skinny85 , I'm interested in contributing aws-cdk. I'm goint to use |
Hey @ymhiroki, that's awesome that you want to contribute this fix! Note one thing: you can't simply change To work around that problem, you will have to introduce a new feature flag for fixing this bug. There are many example PRs you can use to see how other feature flags are implemented (just search the CDK PRs for "feature flag". Here is a simple example. Best of luck! |
…eature flag) fixes aws#18578
Thanks for your kind advice, @skinny85 ! I added the feature flag and try to run the integration tests Do you have any idea about this error and any solutions?
|
So, this is exactly what I mentioned 🙂. You changed the ID of the proxy from the simple ID ( Can you show the code you changed in the |
I pushed the code here. |
Hmm. I wonder if the Can you try setting |
When I set I added the testcase in integ.proxy.ts to pass the CI, however, it fails when I run How can I pass the CI or make the snapshot successfully?
|
I would ask on the PR you opened (#23703), someone from the CDK team should give you the current advice (I'm no longer with Amazon). I would also convert it to a non-draft, I think it's good enough 🙂. I would also take a look at the integration tests section of the Contributing guide. Good luck! |
I'm sorry for my ignorance about your position. I really appreciate your kind comments. |
No need to apologize, you had no way of knowing, and even if you knew, you did absolutely nothing wrong 🙂. |
…under feature flag) (#23703) fixes #18578 ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https:/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:/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https:/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] 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*
|
What is the problem?
The documentation gives the following info for the dbProxy prop:
"Type: string (optional, default: Generated by CloudFormation (recommended))"
However, the dbProxyName is not autogenerated by CFN. Instead the dbProxy name is set to the
id
string if props.dbProxy is not present.Reproduction Steps
Create a DatabaseProxy without specifying a
dbProxyName
.What did you expect to happen?
The
dbProxyName
would be autogenerated by CloudFormation, as stated in the docs.What actually happened?
The
dbProxyName
will be set to theid
of the construct.CDK CLI Version
v1.72.0
Framework Version
No response
Node.js Version
v17.0.1
OS
macOS Big Sur 11.6.2
Language
Typescript
Language Version
No response
Other information
Looks like the following line is causing the issue, but I'd need to dig deeper to see if the fix is this easy
aws-cdk/packages/@aws-cdk/aws-rds/lib/proxy.ts
Line 417 in 3721358
The text was updated successfully, but these errors were encountered: