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

configs/configschema: Introduce the NestingGroup mode for blocks #20949

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

apparentlymart
Copy link
Contributor

In study of existing providers we've found a pattern we weren't previously accounting for of using a nested block type to represent a group of arguments that relate to a particular feature that is always enabled but where it improves configuration readability to group all of its settings together in a nested block.

The existing NestingSingle is not a good fit for this because it is designed under the assumption that the presence or absence of the block has some significance in enabling or disabling the relevant feature, and so for these always-active cases we'd generate a misleading plan where the settings for the feature appear totally absent, rather than showing the default values that will be selected.

NestingGroup is, therefore, a slight variation of NestingSingle where presence vs. absence of the block is not distinguishable (it's never null) and instead its contents are treated as unset when the block is absent. This then in turn causes any default values associated with the nested arguments to be honored and displayed in the plan whenever the block is not explicitly configured.

The current SDK cannot activate this mode, but that's okay because its "legacy type system" opt-out flag allows it to force a block to be processed in this way anyway.


I didn't want to add this in right now, but I couldn't think of a reasonable way to defer adding it without requiring a breaking change later. Therefore I've tested this carefully with both some tests shown in the changeset here and with a special test provider (not using the current SDK) that allowed me to exercise this feature in a more open-ended way. One result of that open-ended testing is shown in the following screenshot:

plan showing a nested block that is populated even though it wasn't set in config

In this test resource type, the access block was defined as NestingGroup and then I ran terraform apply with no explicit block present in configuration. With NestingSingle we would've seen no mention of that block in the plan at all, but now with NestingGroup we can see the default block fully expanded with the default policy value of {}.

This will be essentially dead code in real-world use for the time being, until we release an updated SDK with support for activating this mode. But having the mechanisms in place to deal with it now means it should be safe to release that SDK later without making a breaking change to the plugin protocol.

The diff seems large because I visited every usage of configschema.NestingSingle to determine what would be the suitable behavior for configschema.NestingGroup in that scenario. In many cases, we treat it as equivalent to NestingSingle because the main handling of the NestingGroup behavior happens at decode time.

These helpers determine the value that would be used for a particular
schema construct if the configuration construct it represents is not
present (or, in the case of *Block, empty) in the configuration.

This is different than cty.NullVal on the implied type because it might
return non-null "empty" values for certain constructs if their absence
would be reported as such during a decode with no required attributes or
blocks.
In study of existing providers we've found a pattern we werent previously
accounting for of using a nested block type to represent a group of
arguments that relate to a particular feature that is always enabled but
where it improves configuration readability to group all of its settings
together in a nested block.

The existing NestingSingle was not a good fit for this because it is
designed under the assumption that the presence or absence of the block
has some significance in enabling or disabling the relevant feature, and
so for these always-active cases we'd generate a misleading plan where
the settings for the feature appear totally absent, rather than showing
the default values that will be selected.

NestingGroup is, therefore, a slight variation of NestingSingle where
presence vs. absence of the block is not distinguishable (it's never null)
and instead its contents are treated as unset when the block is absent.
This then in turn causes any default values associated with the nested
arguments to be honored and displayed in the plan whenever the block is
not explicitly configured.

The current SDK cannot activate this mode, but that's okay because its
"legacy type system" opt-out flag allows it to force a block to be
processed in this way anyway. We're adding this now so that we can
introduce the feature in a future SDK without causing a breaking change
to the protocol, since the set of possible block nesting modes is not
extensible.
@apparentlymart apparentlymart merged commit 88e76fa into master Apr 10, 2019
@apparentlymart apparentlymart deleted the f-nested-block-group branch July 10, 2019 14:36
@ghost
Copy link

ghost commented Jul 24, 2019

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 Jul 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants