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

computed_major_engine_version not working for postgres 9.6.18 #82

Open
JCFerrerG opened this issue Dec 17, 2020 · 2 comments
Open

computed_major_engine_version not working for postgres 9.6.18 #82

JCFerrerG opened this issue Dec 17, 2020 · 2 comments
Labels
bug 🐛 An issue with the system

Comments

@JCFerrerG
Copy link

JCFerrerG commented Dec 17, 2020

Describe the Bug

computed major_engine_version fails when...

var.engine == "postgres"
var.engine_version == "9.6.18"

The computed value should be 9.6 but actually results in 9 and the following error:

Error: Error creating DB Option Group: InvalidParameterCombination: Cannot find major version 9 for postgres

Expected Behavior

When var.engine_version == "9.6.18", the computed_major_engine_version should be 9.6.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Copy examples/complete to a new terraform configuration.
  2. Change the following variables in fixtures.us-east-2.tfvars:
engine = "postgresql"
engine_version = "9.6.18"
major_engine_version = "9.6" # or set to empty string
db_parameter_group = "postgres9.6"
  1. Run the plan terraform plan -var-file="fixtures.us-east-2.tfvars". Observe that...
resource "aws_db_option_group" "default" {
     ...
     engine_name              = "postgres"
     major_engine_version     = "9"
     ...
}
  1. Apply the configuration with terraform apply -var-file="fixtures.us-east-2.tfvars
  2. See error

Environment (please complete the following information):

Anything that will help us triage the bug will help. Here are some ideas:

  • OS: OSX
  • cloudeposse/terraform-aws-rds v0.26.0
  • terraform v0.12.29
@JCFerrerG JCFerrerG added the bug 🐛 An issue with the system label Dec 17, 2020
JCFerrerG added a commit to JCFerrerG/terraform-aws-rds that referenced this issue Dec 17, 2020
Without this change, setting `var.major_engine_version` has no effect.

Setting `var.major_engine_version` manually can be used as a workaround for cloudposse#82
@JCFerrerG
Copy link
Author

JCFerrerG commented Dec 17, 2020

Actually, my expectation that the major_engine_version follow a pattern of x.x like 9.6 is inconsistent with the assumptions that were implemented in #42 (which expects that version to be a single number).

Open questions:

  • What is the pattern for major version on DB option groups?
  • Is that pattern consistent for different versions of postgres?

Maybe it is infeasible to calculate major_engine_version consistently?

If that's the case, I might suggest making major_engine_version required and removing the ability to calculate it (as is the case with aws_db_option_group).

For now, I'll be specifying the var.major_engine_version like in this example: JCFerrerG@bfd3151

@antdking
Copy link

antdking commented May 4, 2021

If it's of use, we're using a different way to calculate this internally:

# leave the regex open ended, Aurora, Oracle and SQLServer can all contain characters
version_parts = regex("^(?P<major>[0-9]+)(?:\\.(?P<minor>[0-9]+))?", var.engine_version)
version_parts_number = {
  major = tonumber(local.version_parts["major"])
}
is_postgres                  = var.engine == "postgres"
uses_shorthand_major_version = local.is_postgres && local.version_parts_number["major"] >= 10

computed_major_engine_version = local.uses_shorthand_major_version ? local.version_parts["major"] : "${local.version_parts["major"]}.${local.version_parts["minor"]}"
major_engine_version          = var.major_engine_version == "" ? local.computed_major_engine_version : var.major_engine_version

It's working for postgres 9.6, 11, and 12.
Feel free to use it here.
We're also using this to set aws_db_instance.engine_version, so we don't get state changes whenever an auto-minor update happens.

while the comment talks about other databases, this has only been tested with Postgres. Looking at the docs for sqlserver and oracle, it should still work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An issue with the system
Projects
None yet
Development

No branches or pull requests

2 participants