core + provider/terraform: Fix outputs in remote state #7127
Conversation
The helper/schema framework for building providers previously validated
in all cases that each field being set in state was in the schema.
However, in order to support remote state in a usable fashion, the need
has arisen for the top level attributes of the resource to be created
dynamically. In order to still be able to use helper/schema, this commit
adds the capability to assign additional fields.
Though I do not forsee this being used by providers other than remote
state (and that eventually may move into Terraform Core rather than
being a provider), the usage and semantics are:
To opt into dynamic attributes, add a schema attribute named
"__has_dynamic_attributes", and make it an optional string with no
default value, in order that it does not appear in diffs:
"__has_dynamic_attributes": {
Type: schema.TypeString
Optional: true
}
In the read callback, use the d.UnsafeSetFieldRaw(key, value) function
to set the dynamic attributes.
Note that other fields in the schema _are_ copied into state, and that
the names of the schema fields cannot currently be used as dynamic
attribute names, as we check to ensure a value is not already set for a
given key.
jen20
force-pushed
the
b-remote-state-fix
branch
from
1fab565
to
c85c2fd
Compare
jen20
force-pushed
the
b-remote-state-fix
branch
from
c85c2fd
to
12f36a3
Compare
|
|
||
| var members []string | ||
| for _, key := range keys { | ||
| members = append(members, attributes[key]) | ||
| } | ||
| // This behaviour still seems very broken to me... it retains BC but is | ||
| // probably going to cause problems in future | ||
| sort.Strings(members) |
There was a problem hiding this comment.
We just discussed this in chat - sorting by keys rather than members here is strictly better, and a major release is definitely the time to make this change.
In order to address the BC issues here of list interpolations unexpectedly shuffling on upgrade, we're going to introduce a sort() interpolation function to sort the contents of a list. This should allow users seeing unexpected diffs due to list shuffling on an 0.7 upgrade to retain the old behavior.
There was a problem hiding this comment.
This is implemented in #7128.
jen20
force-pushed
the
b-remote-state-fix
branch
from
12f36a3
to
6e305ef
Compare
This commit makes two changes: map interpolation can now read flatmapped structures, such as those present in remote state outputs, and lists are sorted by the index instead of the value.
The work integrated in #6322 silently broke the ability to use remote state correctly. This commit adds a fix for that, making use of the work integrated in #7124. In order to deal with outputs which are complex structures, we use a forked version of the flatmap package - the difference in the version this commit vs the github.com/hashicorp/terraform/flatmap package is that we add in an additional key for map counts which state requires. Because we bypass the normal helper/schema mechanism, this is not set for us. Because of the HIL type checking of maps, values must be of a homogenous type. This is unfortunate, as it means we can no longer refer to outputs as: ${terraform_remote_state.foo.output.outputname} Instead we had to bring them to the top level namespace: ${terraform_remote_state.foo.outputname} This actually does lead to better overall usability - and the BC breakage is made better by the fact that indexing would have broken the original syntax anyway. We also add a real-world test and assert against specific values. Tests which were previously acceptance tests are now run as unit tests, so regression should be identified at a much earlier stage.
jen20
force-pushed
the
b-remote-state-fix
branch
from
6e305ef
to
bdc6a49
Compare
|
LGTM pending travis. IMO the |
|
Looks like the docs were not updated to reflect the dropping of .output from the data source. https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/terraform/index.html.markdown |
|
@chandy thanks for pointing that out! There is a lot of documentation to update for lists and maps, which I will work on over the remainder of this week and early next week. |
|
you saved all the most fun for last :) |
|
hi guys just wanted to know if this is in the latest release of v 0.7X ? |
|
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
locked and limited conversation to collaborators
The work integrated in #6322 "silently" broke the ability to use remote state correctly. This was widely reported as an issue in Terraform 0.7.0-rc1 both via the mailing list and the HangOps chat.
The commit messages are detailed and should be the primary reference for review: some core work on the helper/schema package was required.
cc: @phinze, @jbardin, @mitchellh