Closed endzyme closed 6 years ago
Change merged https://github.com/reactiveops/terraform-vpc/pull/
In my opinion, TF_VAR_aws* should be removed with a migration. The version of the module should be in auditomation but perhaps it should also get bumped by them migration? I could go either way on. If we bump the version of the module here, then we escape the problem with having the two projects being tightly coupled?
Are you worried that the change in Terraform module will have unexpected consequences and shold not be updated automatically?
Generally speaking, migrations are safe. Nothing irrevocable can be done with a migration. The "worst case" is needing to revert that specific commit.
Here is the workflow I am envisioning, and supporting reasons to my decision to not include a migation.
I can also see people (myself included) making the migration and then not realizing you need to push the new secrets to 1password - just causing confusion (since it's not listed in the git commit delta)
I strongly think we should not add extra work that, in my opinion, will lead to further confusion with migrations.
Could this be prevented by running Terraform more often? Or by adding notes/documentation to the migration? Or informing, up front, those running the migrations that Terraform needs to be run?
It seems to me you are displacing the 'extra work' from here to there and punting it down the road. The change still needs to be made does it not?
Maybe this is something you could outline in a document and put into ROTime since it's outside of the scope for my specific change for SRE work and I honestly don't have the time to tackle the larger problem of why people don't run terraform more often.
Thanks for the feedback.
I'm coming in a little late here but I see a lot of value adding this as a migration. The previous VPC module code breaks terraform import
. Anytime we need to import something we then need to either know to upgrade the VPC module or temporarily remove TF code. Fixing that seems worth including a no-op TF change by migration.
Side note, I would expect anyone running migrations to test terraform plan
before committing code. If they don't that definitely sets up potential for an ugly situation down the road.
@endzyme If this isn't somewhere you can spend time I'd be willing to spend some RO time adding the migration. I would test it on one of our pod repos to make sure it's a clean no-op.
Reopening because I see value in the work and conversation
@davekonopka Thank you for taking this on. Please don't hesitate to reach out with questions or context on what's in play currently. I've also written a migration with pentagon in the past so feel free to ask away on that too.
Thanks for moving this forward!
@davekonopka When you're ready to create one, let me know and I can walk you through it as needed. You should also be able to reference the existing ones to gooed effect.
@ejether @endzyme I merged in master to get the 2.5.0 changes, bumped to 2.6.0, and added a migration.
The migration:
I tested this out on ***** repo. After the migration I got the changes I expected, ran terraform init
and then terraform plan
showed no changes.
I checked a few other repos. All worked for me.
aws_secret/access_key
settings (they aren't needed anymore)The module doesn't need to have settings for provider since they are invoked by the main instance (inherited). I also removed these keys from secrets and other areas as they are not used outside of this module use case.
NOTE: I have not made a migration for the following reasons:
TF_VAR_aws_access_key
orTF_VAR_aws_secret_key
since the project was initialized.secrets.yml
with a migration to remove the settings as well as adjusting the version of the terraform to be auto-bumped to v3.0.0. This should be handled with auditomation.