-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Add resource sums to terraform plan #2458
Add resource sums to terraform plan #2458
Conversation
toChange := 0 | ||
toCreate := 0 | ||
toDestroy := 0 | ||
toDestroyAndCreate := 0 |
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.
You can avoid the local vars by initializing the PlanSums
struct empty here and incrementing its fields in the loop below, since they'll each start at zero.
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.
Ah, good point! 👍 Thanks! I will change that.
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.
Fixed.
Ah yes I've been meaning to do this! @mitchellh is there an alternative strategy for counting these fields using hooks - or is the loop over modules easiest here? |
@phinze I was first looking at hooks actually. I thought the reason for using hooks was because both Although I can imagine that a solution using hooks might look slightly cleaner. |
25b3223
to
c7e315c
Compare
ToChange: 0, | ||
ToCreate: 0, | ||
ToDestroy: 0, | ||
ToDestroyAndCreate: 0, |
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.
These fields default to their zero values, but the extra clarity doesn't hurt. 👍
LGTM! Merge at will. |
I think it'd definitely be cleaner to do this with hooks. You can hook into I'd prefer that, but don't mind if this is merged as a stop-gap until then. |
c7e315c
to
05992df
Compare
05992df
to
b505d15
Compare
@mitchellh Agreed, to be honest, I just struggled to find the right hook. It's now reworked & using Will anyone of you two give a final 👍 / 👎 , please? :) |
Ooo much nicer. LGTM |
Add resource sums to terraform plan
Much cleaner :) |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Closes #2355
Test plan
sample.tf
$ terraform plan # 2 to add, 0 to change, 0 to destroy
$ terraform plan -target=aws_vpc.main # 1 to add, 0 to change, 0 to destroy
$ terraform plan -destroy # 0 to add, 0 to change, 0 to destroy
$ terraform plan -destroy # 0 to add, 0 to change, 2 to destroy
$ terraform plan -destroy -target=aws_instance.web # 0 to add, 0 to change, 1 to destroy