feat(neptune): high level constructs for db clusters and instances #12763
Conversation
- This change adds higher level constructs for Neptune clusters - Adds higher-level constructs for - AWS::Neptune::DBCluster - AWS::Neptune::DBInstance - AWS::Neptune::DBClusterParameterGroup - AWS::Neptune::DBParameterGroup - AWS::Neptune::DBSubnetGroup fixes aws#12762
|
|
There was a problem hiding this comment.
Wow, thanks for this contribution!
It's going to take me a few days to have the time to dive deep into this, but I've given it a quick skim and it looks like it's in really great shape. I imagine this is heavily inspired/borrowed from the RDS module.
One point of note is that the description claims this provides the high-level constructs for clusters and instances, but I'm only seeing "L2s" for the cluster. That's totally fine; this is big enough as-is, and we should be able to add in the instances later. I just wanted to clarify intent.
Another minor high-level note is that the pattern of cluster-ref and cluster is kind of a "legacy" holdover and I'd rather us just combine into one file. Same with props, although I can imagine the instance-specific stuff there going into the (eventual) instance L2.
Lastly, we'll want to add more detail to the README to cover some of the other usage/elements. It doesn't need to be 100% comprehensive, but a bit more than is currently there I think is necessary to get folks up and running quickly.
I'll circle back around (hopefully) next week to do the first pass on the code itself.
Thanks again!
|
Thanks Nick for taking a quick pass at this (yes, code is inspired (read: copied 🙂) from rds/docdb modules)! Good point regarding the instances - I was initially thinking of adding a L2 construct for that but didn't end up doing it because most customers just pick the no. of instances and let the service figure out the distribution. But it might be worth adding if someone wants to give specific names to instances or launch them in specific availability zones. I will take a stab at this, will get rid of current |
mergify
bot
dismissed
njlynch’s stale review
Pull request has been modified.
| * | ||
| * @default - false, connections to database don't require IAM auth | ||
| */ | ||
| readonly iamAuthEnabled?: boolean; |
There was a problem hiding this comment.
I wanted to test this before adding support for it, found some conflicting information where AWS CLI says IAM authentication is not supported by Neptune at the cluster level but is supported at instance level (whereas CFN has no flag to pass this value at instance level). Wanted to check if SDK documentation is incorrect or if it is a really an instance specific setting and if so whether CFN support for IAM needs to be fixed.
|
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 |
fixes #12762
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license