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

Extend attr.Value interface to support IsFullyNullableKnown() #980

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

magodo
Copy link

@magodo magodo commented Apr 16, 2024

This new method is similar to the Value.IsFullyKnown() that is available in the github.com/hashicorp/terraform-plugin-go/tftypes.

The difference here is that in tftypes, each value can only has two states: a concrete value (including nil) or "unknown". While in the fw, each value can has three states: null, unknown and known. This is why the method name is chose so (as I can't figure out another better name, as IsFullyNotKnown or IsPartiallyUnknown are ambiguous than the current one, IMO).

The reason for introducing this method is to allow provider developers to check the state of an aggregate value during the ModifyPlan, where the code might stop processing that property if its value contains any unknwon value. Currently, the developer has two solutions:

  • Convert the ToTerraformValue() to convert the attr.Value to tftypes.Value, then call its IsFullyKnown(). This works fine (and it is also used in the FW itself somewhere), while it is a bit over kill to do the conversion where the intent is only to check the whole (un)known-ness.

  • Self implement the IsFullyKnown() for the attr.Value, similar to:

    func IsFullyKnown(val attr.Value) bool {
            if val == nil {
                    return true
            }
            if val.IsUnknown() {
                    return false
            }
            switch v := val.(type) {
            case types.Dynamic:
                    return IsFullyKnown(v.UnderlyingValue())
            case types.List:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Set:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Tuple:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Map:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Object:
                    for _, e := range v.Attributes() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            default:
                    return true
            }
    }

This PR tries to put this common logic to the FW so that more developers can save the run/develop time effort for the same purpose. I chose to extend the attr.Value interface, instead of introducing a helper method in the attr package, as a random choice. If the latter looks better, then I can rework this PR.

This new method is similar to the `Value.IsFullyKnown()` that is available in the `github.com/hashicorp/terraform-plugin-go/tftypes`.

The difference here is that in `tftypes`, each value can only has two states: a concrete value (including `nil`) or "unknown". While in the fw, each value can has three states: null, unknown and known. This is why the method name is chose so (as I can't figure out another better name, as `IsFullyNotKnown` or `IsPartiallyUnknown` are ambiguous than the current one, IMO).

The reason for introducing this method is to allow provider developers to check the state of an aggregate value during the `ModifyPlan`, where the code might stop processing that property if its value contains any unknwon value. Currently, the developer has two solutions:

- Convert the `ToTerraformValue()` to convert the `attr.Value` to `tftypes.Value`, then call its `IsFullyKnown()`. This works fine (and it is also used in the FW itself somewhere), while it is a bit over kill to do the conversion where the intent is only to check the whole (un)known-ness.
- Self implement the `IsFullyKnown()` for the `attr.Value`, similar to:

    ```go
    func IsFullyKnown(val attr.Value) bool {
            if val == nil {
                    return true
            }
            if val.IsUnknown() {
                    return false
            }
            switch v := val.(type) {
            case types.Dynamic:
                    return IsFullyKnown(v.UnderlyingValue())
            case types.List:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Set:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Tuple:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Map:
                    for _, e := range v.Elements() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            case types.Object:
                    for _, e := range v.Attributes() {
                            if !IsFullyKnown(e) {
                                    return false
                            }
                    }
                    return true
            default:
                    return true
            }
    }
    ```

This PR tries to put this common logic to the FW so that more developers can save the run/develop time effort for the same purpose. I chose to extend the `attr.Value` interface, instead of introducing a helper method in the `attr` package, as a random choice. If the latter looks better, then I can rework this PR.
@magodo magodo requested a review from a team as a code owner April 16, 2024 06:34
@bflad
Copy link
Member

bflad commented Apr 16, 2024

Related feature request issue: #597

For any reviewers, please note that the current attr.Value interface change is a breaking change since it would force all existing custom type implementations to need to include the new method. I'm adding the GitHub label to call this out. @magodo I'm not providing a full review, but it might be good to update the proposed implementation to avoid the breaking change so it potentially can get into any release, rather than needing to wait for a future major version of the Go module. If you would like to discuss available options there, please reach out.

@bflad bflad added enhancement New feature or request breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. labels Apr 16, 2024
@magodo
Copy link
Author

magodo commented Apr 17, 2024

@bflad The other possible option I can think of is the 2nd one I mentioned above, which won't handle the custom types. I don't know how can we avoid breaking change while still taking custom types into consideration at this moment..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants