-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
r/ecs_task_definition: Fix equivalency comparator #2339
Conversation
I'm happy to test - how do I go about doing that? |
@ziggythehamster Try cloning the repository and building the binary locally: https://github.com/terraform-providers/terraform-provider-aws#building-the-provider then in the directory with your ECS configs just Eventually I can try and build it for your platform if you tell me more about your environment - i.e. architecture & OS. |
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.
👍 one logging nit/suggestion
canonicalJson2, err := jsonutil.BuildJSON(obj2) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
log.Printf("[DEBUG] Comparing canonical definitions,\nFirst: %s\nSecond: %s\n", | ||
canonicalJson1, canonicalJson2) |
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.
nit: not sure if this is debugging that you left in unintentionally, however if intended, maybe we should log instead the result of the compare; logical match
or difference
etc. Right here we're just logging both and not actually saying if Terraform seems them as equal, that's left up to the user debugger
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.
right, good point - I'm happy to add more context (i.e. whether it's a match).
The reason I left it there is because I had hard times comparing those JSONs in a failing test, where you can only get the original (non-canonical) JSONs which is also kind of intention, because this process should remain hidden as implementation detail.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Closes #2336
@ziggythehamster @nathanielks @achille-roussel @bturbes - do you folks mind testing this with your configs and/or pasting it here so we can test it to make sure the bug doesn't re-appear?
Test results