-
Notifications
You must be signed in to change notification settings - Fork 36
Fix s3 backend setting DynamoDB state lock on by default #77
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
Conversation
bentsku
left a comment
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.
Awesome, thanks a lot for your contribution! The change looks sound, I'm just a bit worried into setting default values for versions of Terraform that don't support use_lockfile yet.
After looking more into S3 Backend State Locking, it seems it is actually an opt-in behavior in Terraform: https://developer.hashicorp.com/terraform/language/backend/s3#state-locking
I'm not sure why we are setting the DynamoDB locking on by default, but do you think it would actually be better to just remove it (as it is opt-in, it should not be "on" by default? ) and let users decide if they want to use it?
We could keep the custom logic you've modified to actually create the DynamoDB table if it is configured by the user, but otherwise just not set it on by default? It doesn't seem to be something needed anyway when working in local mode.
I'd like to hear if that would also solve your issue, and if you think that would be an acceptable solution?
|
Yeah that seems like a completely reasonable idea, only include them if the user is providing them, otherwise we just run with no locking, as really it shouldn't be needed in a local setup. I'll get those changes up to, and fix the overzealous linting 🫠 |
|
That's been changed up @bentsku, good call, looks much more elegant. |
bentsku
left a comment
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.
LGTM! Thanks for addressing the comments, I'll push the update/readme changes and merge/release a new version 😄 thanks a lot for bringing this discussion up!
|
@hubbcaps I've updated the readme/bumped the version, and as I was there, quickly added a test for it on the branch, hope that's okay 😄 |
|
Absolutely dude, testing is always good 😁 |
For some reason my dynamoDB config is constantly being clobbered and even with the creation of the default lock table, I keep getting non existent table errors from tflocal, and it's blocking my on boarding as a new enterprise customer.
Since DynamoDB is deprecated anyways, here's support for
use_lockfilewhich is the new standard and simply uses an object in the state s3 bucket to handle locking.This PR switches to use_lockfile as the new standard config, and if someone wants to override it with a bucket config, we yank out use_lockfile and throw their table into the previous logic path of get_or_create on the table they have configured.
Let me know if there's any fixes you'd like added, but I think its pretty solid.
https://developer.hashicorp.com/terraform/language/backend/s3#enabling-s3-state-locking