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

[WIP]: Add failing test for missing computed map entries #9634

Merged
merged 12 commits into from
Nov 9, 2016

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 26, 2016

Failing test fixture for #9549

The map output from the module "mod" loses the computed value from the
template when we validate. If the "extra" field is removed from the map,
the validation fails earlier with map "does not have any elements so
cannot determine type".

Apply will work, because the computed value will exist in the map.

return UnknownVariableValue, nil
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the critical section. Because HIL (on f-unknown, so travis will fail) handles all this automatically for us now, we don't have to do any special handling. We just evaluate HIL like normal below...

@jbardin
Copy link
Member Author

jbardin commented Oct 27, 2016

This looks great! I thought I could get this without going into HIL, but ended up going in circles.

Is it possible now that we no longer need the config.UnknownVariableValue uuid with TypeUnknown?

If you want to do the HIL PR, I can take care of cleaning up this PR and fixing the vendoring.

@mitchellh
Copy link
Contributor

mitchellh commented Oct 27, 2016

Perfect, made the HIL PR here: hashicorp/hil#31

EDIT: Yes, we can actually remove config.UnknownVariableValue. HURRAY! It looks like we just moved the problem elsewhere, but this is actually a really big deal because we have a lot more control over that stuff now.

@jbardin
Copy link
Member Author

jbardin commented Oct 27, 2016

We might just to do the work to pull the UnknownVariableValue out now, as it's leaking into a few tests.

Interestingly, some helper schema tests are failing non-deterministically, with varying diffs and arguments to Schema.Set.

@mitchellh
Copy link
Contributor

Picking this up for 0.8, looking into test failures.

@mitchellh mitchellh force-pushed the jbardin/GH-9549 branch 2 times, most recently from f4a75ce to 3a9e68c Compare November 9, 2016 01:17
jbardin and others added 12 commits November 9, 2016 14:28
The map output from the module "mod" loses the computed value from the
template when we validate. If the "extra" field is removed from the map,
the validation fails earlier with map "does not have any elements so
cannot determine type".

Apply will work, because the computed value will exist in the map.
This makes all the computed stuff "just work" since HIL uses the same
computed sentinel value (string UUID) and the type differentiates it
from a regular string.
The primary change here is to expect that Config contains computed
values. This introduces `unknownCheckWalker` that does a really basic
reflectwalk to look for computed values and use that for IsComputed.

We had a weird mixture before checking whether c.Config was simply
missing values to determine where to look. Now we rely on IsComputed
heavily.
@mitchellh mitchellh merged commit b93331b into master Nov 9, 2016
@mitchellh mitchellh deleted the jbardin/GH-9549 branch November 9, 2016 22:33
@ghost
Copy link

ghost commented Apr 20, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants