hashicorp / terraform-provider-vault

Terraform Vault provider
https://www.terraform.io/docs/providers/vault/
Mozilla Public License 2.0
457 stars 536 forks source link

Update max db conn #2119

Closed conor-mccullough closed 2 weeks ago

conor-mccullough commented 8 months ago

Description

The default value in our docs is 4, I'm not sure if there is a good reason to keep the providers default as 2. In my mind it makes sense to keep the same limit across different implementation techniques, unless there's a justification for this being lower in the provider.

Checklist

Output from acceptance testing:

ok      github.com/hashicorp/terraform-provider-vault/vault 1.862s

This is a pretty basic change so I haven't done super extensive testing. Let me know if this needs more stringent tests performed, happy to resubmit.

Community Note

fairclothjm commented 8 months ago

@conor-mccullough Thanks for the submission! It looks like a few tests are failing due to the previously expected default value

TestAccDatabaseSecretBackendConnection_postgresql_import
TestAccDatabaseSecretBackendConnection_mssql
TestAccDatabaseSecretBackendConnection_mysql
TestAccDatabaseSecretBackendConnectionUpdate_mysql
TestAccDatabaseSecretBackendConnection_postgresql
TestAccDatabaseSecretsMount_mssql
conor-mccullough commented 7 months ago

Hi @fairclothjm

Apologies for the delay here. I was in the process of testing this, and had also implemented your additional feedback from Slack in the new change:

I am wondering if we should instead make this a computed value so that we don’t have to update the TFVP code if the Default ever changes in Vault?

However due to its low priority/impact relative to some more-than-usual firefighting over the last month, it keeps getting pushed to the bottom of my list. I hope to get to it in the next month assuming things calm a bit more.