Skip to content
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

Fix #16: issue with removing folders #20

Merged
merged 1 commit into from
Nov 5, 2019
Merged

Conversation

morgante
Copy link
Contributor

@morgante morgante commented Nov 5, 2019

Fixes #16 by slicing the outputs to the length of inputs.

@ludoo
Copy link
Contributor

ludoo commented Nov 5, 2019

This is a fix, but there's a potential corner case. State has resources matching

names = ["one", "two", "three"]

and then change to

names = ["one", "three"]

and run terraform output. The outputs will be incorrect imho. I would prefer an error that forces removing outputs until state matches vars. What do you think?

@ludoo
Copy link
Contributor

ludoo commented Nov 5, 2019

@averbuks read my comment above...

@morgante
Copy link
Contributor Author

morgante commented Nov 5, 2019

The outputs will be incorrect imho.

Are you sure?

Only until/unless you run terraform apply. I don't think there should be any expectation that a Terraform config with un-applied changes produces reasonable results.

In fact, it would be the same outcome even without this. This is truncating the list, but the out

and run terraform output. The outputs will be incorrect imho. I would prefer an error that forces removing outputs until state matches vars. What do you think?

We should not force people to edit module internals simply to apply a perfectly valid config.

@morgante morgante merged commit 53edd09 into master Nov 5, 2019
@ludoo
Copy link
Contributor

ludoo commented Nov 5, 2019

I misinterpred the order of execution, so disregard my previous comments! Thanks @morgante for pointing that out privately :)

@ludoo
Copy link
Contributor

ludoo commented Nov 9, 2019

I mulled this over a bit as there are similar issues in the service accounts module, and there's something I can't wrap my head around.

Removing an element from the names variable triggers output errors in zipmaps as the resource length in state does not match names length, and this I get of course (and slicing fixes it).

What I don't get is why a similar error isn't raised when an element is added to the names variable. The outputs length should still be mismatched vs name length, and slicing it should of course make no difference, but I don't seem to be able to reproduce this case.

Can someone who understands Terraform internals shed some light here? Is the evaluation only done when destroying resources, which resonates with the issues open in the Terraform repository on this subject, that mostly seem to deal with destroy operations?

@aaron-lane aaron-lane deleted the bugfix/delete-folder branch November 12, 2019 18:49
@aaron-lane
Copy link
Contributor

@ludoo what is the output in the scenario of adding an extra name?

@ludoo
Copy link
Contributor

ludoo commented Nov 12, 2019

@ludoo what is the output in the scenario of adding an extra name?

What you'd expect after a successful apply. The issue is only when deleting a resource, either through a change in inputs or when destroying, so Morgante's patch addresses it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in outputs when removing folders
4 participants