-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
feat: Fix serverless v2 engine and add missing resource arguments #317
feat: Fix serverless v2 engine and add missing resource arguments #317
Conversation
Would there be a rough estimate on when this (esp the fix for #313 ) would be merged? 🙏 😅 |
12b28b5
to
a1f4b57
Compare
a1f4b57
to
90578b2
Compare
resource "aws_rds_cluster" "this" { | ||
count = local.create_cluster ? 1 : 0 | ||
|
||
# Notes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of noise due to the re-ordering to match the AWS provider (makes it easier to compare what we support vs what we need to add), apologies for that. However, nothing was deleted - only re-order or the addition of new arguments that we did not previously support (mostly for RDS multi-AZ plus a few others)
|
||
engine = var.engine | ||
engine_mode = var.engine_mode | ||
engine_version = local.is_serverless ? null : var.engine_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was fixed for the serverless v1 upgrade so users can now set the engine version - not sure when this changed but now its configurable for #313 (i.e. - the conditional check was removed and now its just engine_version = var.engine_version
)
} | ||
|
||
dynamic "scaling_configuration" { | ||
for_each = length(keys(var.scaling_configuration)) == 0 || !local.is_serverless ? [] : [var.scaling_configuration] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of these for_each
conditionals were cleaned up and simplified
resource "aws_rds_cluster_endpoint" "this" { | ||
for_each = local.create_cluster && !local.is_serverless ? var.endpoints : tomap({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed the looping comprehension
} | ||
|
||
resource "aws_rds_cluster_instance" "this" { | ||
for_each = local.create_cluster && !local.is_serverless ? var.instances : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed looping comprehension
resource "aws_rds_cluster_role_association" "this" { | ||
for_each = local.create_cluster ? var.iam_roles : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed looping comprehension
|
||
db_cluster_identifier = try(aws_rds_cluster.this[0].id, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of these wrapped try()
s are not doing anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment. LGTM.
Co-authored-by: Anton Babenko <anton@antonbabenko.com>
Co-authored-by: Anton Babenko <anton@antonbabenko.com>
## [7.4.0](v7.3.0...v7.4.0) (2022-09-07) ### Features * Fix serverless v2 engine and add missing resource arguments ([#317](#317)) ([2d87320](2d87320))
This PR is included in version 7.4.0 🎉 |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
availability_zones
,db_cluster_instance_class
,iops
,storage_type
, andallocated_storage
to support RDS multi-AZcluster_identifier_prefix
by addingcluster_use_name_prefix
variablecluster_members
argument/variableengine_mode == "provisioned"
for v2 modeengine_version
when using serverless v1 (currently forcesnull
but now can be configured by users)v1
andv2
for which serverless mode is usedMotivation and Context
multi_az
#315Breaking Changes
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectsserverless
examplemulti-az
examplepostgresql
examplepre-commit run -a
on my pull request