seqeralabs / terraform-seqera-aws

Apache License 2.0
5 stars 3 forks source link

Rename DB variables #9

Closed pditommaso closed 10 months ago

pditommaso commented 10 months ago

Let's rename those variables:

pditommaso commented 10 months ago

Rationale:

enekui commented 10 months ago

Please, note that the naming convention that I am using is to keep consistency with AWS and Terraform RDS database resources. Also, since we are using official AWS modules, I keep the same naming convention that they use in the module parameters to keep consistency across all the code. This is a best practice that allow us to keep the code clean and easy to debug, scale, and support. As an example of that: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/rds_cluster#master_password https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/rds_cluster#master_username And I add the db prefix since this is the relevant to our module. Same for AWS console, and API.

image

Kindly note that this module is intended to be publicly used, so we need to take care of all the Terraform and AWS good practices as we can see in other AWS terraform modules. You can see here, in the Terraform RDS module that we are using how they use db_name, we need to be consistent with it since it's the official AWS module that we are using to power our module. https://github.com/terraform-aws-modules/terraform-aws-rds#input_db_name I truly hope this explanation can clarify the reasoning behind the naming convention that I am using.

pditommaso commented 10 months ago

Note, we'll need to make this available also for GKE and and Azure. Each of those will have their own convention, so we need to have own convention

enekui commented 10 months ago

Note, we'll need to make this available also for GKE and and Azure. Each of those will have their own convention, so we need to have own convention

Yes, but this module can't handle another cloud. For Azure and Google, we need to create different modules.

pditommaso commented 10 months ago

Indeed, but I think we should not use a different set of variables across those modules.

enekui commented 10 months ago

Indeed, but I think we should not use a different set of variables across those modules.

At the end of the day, I will do what you say. But note that the variable naming should always be driven by the underlying module, and by the naming convention of the cloud provider. If you want us to use those variables you mentioned above, I will do it. There is no point in making this thread longer.

pditommaso commented 10 months ago

But note that the variable naming should always be driven by the underlying module

Don't agree on this, also because the author of the module we have own conventions

enekui commented 10 months ago

Done. Just changed db_app_schema to db_app_schema_name to be more consistent with variable naming.