Fix planning. #176
Fix planning. #176
Conversation
There was a problem hiding this comment.
As discussed out of band, this type of change raises some questions around practitioner and provider developer experiences. I would personally love if we could spend some time working through those scenarios, if possible.
My understanding of the current behavior in the new framework is that it is similar to Terraform Plugin SDK version 2, where provider developers must explicitly opt-in to setting new or unknown values via plan modifications for a Computed-only attribute if it is to change from the current state value.
If correctly done by the provider developer, this will show up correctly in terraform plan output, such as OLD => NEW or OLD => (known after apply).
If not, they (provider developers and practitioners alike) will receive the Provider produced inconsistent result after apply error diagnostic from Terraform CLI. (Caveat: This error is demoted to warning logs with SDKv2 when the attribute is not referenced in another location, which is not also updated.)
Since the new framework has all the information that Terraform CLI uses to determine this error condition, it is possible that the framework could raise its own error diagnostic with its own guidance.
My understanding of the proposed behavior with this change is that the framework will by default mark all Computed-only attributes in the plan as unknown. Provider developers would need to explicitly opt-out of the behavior by setting the expected value (even if it is the current state value) via plan modifications.
If done correctly by provider developers, unchanged attributes will show no difference in terraform plan output.
If not, the terraform plan output will always show OLD => (known after apply) for every output or other value references.
The Provider produced inconsistent result after apply error diagnostic is much harder or not possible to induce in this scenario.
In either case, it might be worth writing up some canonical documentation on the manner since it is likely confusing either which way, both for (future) maintainers of this project and provider developers. 👍
tfsdk/schema.go
Outdated
| // list, set, and map nested attributes all return a | ||
| // map[string]Attribute when the path points to an element in the list, | ||
| // set, or path. We need it to point to an attribute specifically, so | ||
| // we're choosing to use the parent attribute in this case, which is | ||
| // usually what we want. | ||
| if len(path.Steps()) > 1 { | ||
| lastStep := path.Steps()[len(path.Steps())-1] | ||
| if _, ok := lastStep.(tftypes.AttributeName); !ok { | ||
| return s.AttributeAtPath(path.WithoutLastStep()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The current method documentation states:
// AttributeAtPath returns the Attribute at the passed path. If the path points
// to an element or attribute of a complex type, rather than to an Attribute,
// it will return an ErrPathInsideAtomicAttribute error.
While this code change seems to be introducing different behavior to that. Which is correct? I think an error is more correct in this situation, rather than silently returning the results of a technically different path.
Separately, we have seen this logic a few times:
path.Steps()[len(path.Steps())-1]Should we consider adding a LastStep() method onto tftypes.AttributePath?
tfsdk/serve_test.go
Outdated
| for _, d := range got.Diagnostics { | ||
| t.Log(d.Detail) | ||
| } |
There was a problem hiding this comment.
Is this something we should be doing everywhere? I'm guessing this was added for debugging the case of go-cmp hiding the full detail string in its difference output, which has definitely been a development annoyance for me when wanting to see it.
There was a problem hiding this comment.
I've added these helpful debugging bits to #135 (comment) so we can decide how we want to handle them going forward. Just to reduce the diff of this pull request, I'm going to extract them out and we can do them consistently across the board at a later time.
This fixes #175. Essentially, our behavior did exactly what we thought it did, but didn't combine with Terraform in the ways we thought it would. We need to set all computed attributes that are null in the _config_ to unknown, not all computed attributes that are null in the _plan_ to unknown, because once a computed attribute has a value in state, it will not have a null plan again unless the provider modifies the plan to null explicitly. This means once a computed attribute gets a value, under the existing code, it can't be updated by the provider. As part of this change, I moved when the null->unknown transform happens. It used to happen at the very end, which gave provider developers no opportunity to override the auto-unknown values with values the provider may know at plan time, such as a provider-level default or the combination of other fields, etc. By doing this first, the provider has the ability to understand that the value is unknown and replace it with a known value. I also fixed a bug that would return an error when trying to use Schema.AttributeAtPath with a path that pointed to an element inside a list, map, or set nested attribute. I also fixed plan modification to still run resource-level plan modification even when the plan is null (i.e., the resource is being deleted) so that diagnostics can be returned. This is useful, for example, when a resource _can't_ be deleted on the API, but the provider is going to just remove it from Terraform's state. This allows provider developers to surface a warning at plan time to practitioners about this limitation, letting them know _before_ they apply the change that that is what's going to happen. Finally, some tests had nonsensical inputs to them that Terraform could never produce--a null config that still had planned values. I updated those tests to have more realistic values by removing the plan and removing the RequiresReplace set in the response, as a deleted resource needing to be replaced doesn't super make sense.
It will likely get documented and consistently applied later either as-is or using a custom go-cmp formatter. See also #135.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This fixes #175. Essentially, our behavior did exactly what we thought
it did, but didn't combine with Terraform in the ways we thought it
would. We need to set all computed attributes that are null in the
config to unknown, not all computed attributes that are null in the
plan to unknown, because once a computed attribute has a value in
state, it will not have a null plan again unless the provider modifies
the plan to null explicitly. This means once a computed attribute gets a
value, under the existing code, it can't be updated by the provider.
As part of this change, I moved when the null->unknown transform
happens. It used to happen at the very end, which gave provider
developers no opportunity to override the auto-unknown values with
values the provider may know at plan time, such as a provider-level
default or the combination of other fields, etc. By doing this first,
the provider has the ability to understand that the value is unknown and
replace it with a known value.
I also fixed a bug that would return an error when trying to use
Schema.AttributeAtPath with a path that pointed to an element inside a
list, map, or set nested attribute.
I also fixed plan modification to still run resource-level plan
modification even when the plan is null (i.e., the resource is being
deleted) so that diagnostics can be returned. This is useful, for
example, when a resource can't be deleted on the API, but the provider
is going to just remove it from Terraform's state. This allows provider
developers to surface a warning at plan time to practitioners about this
limitation, letting them know before they apply the change that that
is what's going to happen.
Finally, some tests had nonsensical inputs to them that Terraform could
never produce--a null config that still had planned values. I updated
those tests to have more realistic values by removing the plan and
removing the RequiresReplace set in the response, as a deleted resource
needing to be replaced doesn't super make sense.