busser / tfautomv

Generate Terraform moved blocks automatically for painless refactoring
https://github.com/busser/tfautomv
Apache License 2.0
682 stars 24 forks source link

tfautomv fails to find move for aws_sqs_queue #53

Open FalconerTC opened 1 year ago

FalconerTC commented 1 year ago

Hello! I like this project and would like to give some data to help improve it. There's a scenario where I'd like to be able to use tfautomv.

  # aws_sqs_queue.backup_queues["statstracking-v3-test-notification-backup"] will be created
  + resource "aws_sqs_queue" "backup_queues" {
      + arn                               = (known after apply)
      + content_based_deduplication       = false
      + deduplication_scope               = (known after apply)
      + delay_seconds                     = 0
      + fifo_queue                        = false
      + fifo_throughput_limit             = (known after apply)
      + id                                = (known after apply)
      + kms_data_key_reuse_period_seconds = (known after apply)
      + max_message_size                  = 262144
      + message_retention_seconds         = 1209600
      + name                              = "statstracking-v3-test-notification-backup"
      + name_prefix                       = (known after apply)
      + policy                            = (known after apply)
      + receive_wait_time_seconds         = 0
      + redrive_allow_policy              = (known after apply)
      + redrive_policy                    = (known after apply)
      + sqs_managed_sse_enabled           = (known after apply)
      + url                               = (known after apply)
      + visibility_timeout_seconds        = 30
    }

  # aws_sqs_queue.statstracking_notification_queue_backup will be destroyed
  # (because aws_sqs_queue.statstracking_notification_queue_backup is not in configuration)
  - resource "aws_sqs_queue" "statstracking_notification_queue_backup" {
      - arn                               = "arn:aws:sqs:eu-west-1:REDACTED:statstracking-v3-test-notification-backup" -> null
      - content_based_deduplication       = false -> null
      - delay_seconds                     = 0 -> null
      - fifo_queue                        = false -> null
      - id                                = "https://sqs.eu-west-1.amazonaws.com/REDACTED/statstracking-v3-test-notification-backup" -> null
      - kms_data_key_reuse_period_seconds = 300 -> null
      - max_message_size                  = 262144 -> null
      - message_retention_seconds         = 1209600 -> null
      - name                              = "statstracking-v3-test-notification-backup" -> null
      - receive_wait_time_seconds         = 0 -> null
      - sqs_managed_sse_enabled           = false -> null
      - url                               = "https://sqs.eu-west-1.amazonaws.com/REDACTED/statstracking-v3-test-notification-backup" -> null
      - visibility_timeout_seconds        = 30 -> null
    }

Here I have tried to refactor a resource to use a for_each. The correct move command is this

moved {
  from = aws_sqs_queue.statstracking_notification_queue_backup
  to   = aws_sqs_queue.backup_queues["statstracking-v3-test-notification-backup"]
}

It seems like tfautomv should be able to pick this up because the queue has the same name in each.

edit: Running the analysis shows the mismatch failed due to changes to the tags. I see in the README I can ignore changes to certain attributes, but it would be nice to have a way of running that identified a move based only on the "primary" attribute of a resource. In this case, two queues with the same name is desirable whereas changes to tags is part of the refactor.

│ │ Mismatch: aws_sqs_queue.statstracking_notification_queue_backup
│ │ ╷
│ │ │ + tags_all.Terraformed = "True"
│ │ │ - tags_all.Terraformed = <nil>
│ │ │ + tags_all.Environment = "Dev"
│ │ │ - tags_all.Environment = "test"
│ │ │ + tags_all.Source = "REDACTED"
│ │ │ - tags_all.Source = <nil>

I see the above even when running tfautomv -terraform-bin=terragrunt -ignore="everything:aws_sqs_queue:tags_all" -ignore="everything:aws_sqs_queue:tags"

busser commented 1 year ago

Thank you for opening this issue! Indeed, the best way to improve tfautomv is through studying cases such as yours.

The current way attribute comparison works is by flattening them into a map with no nesting. Since a single rule can only concern a single attribute — eg. a single tag — there is currently no way to tell tfautomv to ignore all tags. This makes me think that it would be nice to have some form of wildcard to match a subset of attributes. Something like all_tags.*. I am a fan of this, although the exact semantics would need to be thought through.

You mentioned another interesting idea: ignoring all attributes except a single one — the resource's ID, in essence. This could be a new rule and should be fairly straightforward to implement. Something like everything_except:my_resource:my_attribute. Again, the exact semantics would require some thought.

I am currently in the process of rewriting tfautomv's internals in order to find moves spanning different working directories. Once that is done, which should be soon, then I will gladly discuss implementing the two features detailed above. Starting work on them beforehand would yield too many conflicts, so I don't think it's worth it.

In the mean time, I would like to know what you think of these feature ideas. Do you think they would solve your problem? Do you see any ways we could improve these ideas, or any pitfalls we should avoid?