Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

configs: explicitly nullable variable values #29832

Merged
merged 5 commits into from
Nov 1, 2021
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 29, 2021

The current behavior of module input variables is to allow users to
override a default by assigning null, which works contrary to the
behavior of resource attributes, and prevents explicitly accepting a
default when the input must be defined in the configuration.

Add a new variable attribute called nullable which will allow explicitly
defining when a variable can be set to null or not. The current default
behavior is that of nullable=true.

Setting nullable=false in a variable block indicates that the variable
value can never be null. This either requires a non-null input value, or
a non-null default value. In the case of the latter, we also opt-in to
the new behavior of a null input value taking the default rather than
overriding it.

In a future language edition where we make nullable=false the default,
setting nullable=true will allow the legacy behavior of null
overriding a default value. The only future configuration in which this
would be required even if the legacy behavior were not desired is when
setting an optional+nullable value. In that case default=null would
also be needed and we could therefor imply nullable=true without
requiring it in the configuration.

The possible combinations of variable configuration are:

# Variable must be set, but may be explicitly set to null.
# In this case, the module author must deal with var.a possibly being null
variable "a" {
  nullable = true
}

# Variable may be omitted, in which case it takes on the default, or may be
# explicitly set to null in which case it appears as null
# In this case, the module author must deal with var.b possibly being null
variable "b" {
  nullable = true
  default  = "hello"
}

# Optional variable where null is allowed. `nullable=true` is default now, and
# may be inferred in the future from `default=null` value when `nullable=false`
# becomes the default, so setting nullable here should never be required.
# In this case, the module author must deal with var.b possibly being null
variable "c" {
  nullable = true
  default  = null
}

# Non-nullable with a default: if left unset, or set explicitly to null, then
# it takes on the default value
# In this case, the module author can safely assume var.d will never be null
variable "e" {
  nullable = false
  default  = "hello"
}

# Non-nullable with no default: variable must be set, and cannot be null.
# In this case, the module author can safely assume var.d will never be null
variable "e" {
  nullable = false
}

# Non-nullable with a null default: an invalid combination, because a non-nullable
# variable can never be null
variable "f" {
  nullable = false
  default  = null
}

Fixes #24142

@jbardin jbardin requested a review from a team October 29, 2021 17:56
@jbardin jbardin self-assigned this Oct 29, 2021
The current behavior of module input variables is to allow users to
override a default by assigning `null`, which works contrary to the
behavior of resource attributes, and prevents explicitly accepting a
default when the input must be defined in the configuration.

Add a new variable attribute called `nullable` will allow explicitly
defining when a variable can be set to null or not. The current default
behavior is that of `nullable=true`.

Setting `nullable=false` in a variable block indicates that the variable
value can never be null. This either requires a non-null input value, or
a non-null default value. In the case of the latter, we also opt-in to
the new behavior of a `null` input value taking the default rather than
overriding it.

In a future language edition where we make `nullable=false` the default,
setting `nullable=true` will allow the legacy behavior of `null`
overriding a default value. The only future configuration in which this
would be required even if the legacy behavior were not desired is when
setting an optional+nullable value. In that case `default=null` would
also be needed and we could therefor imply `nullable=true` without
requiring it in the configuration.
@jbardin jbardin force-pushed the jbardin/nullable-variable branch from 2a34f33 to f0a64eb Compare October 29, 2021 17:59
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

I didn't have time yet to read through all of this because I have a meeting shortly but I wanted to just share the one comment I already left inline here, in case it's helpful in the meantime, and then I'll come back and read more of this a bit later if someone else doesn't get there first!

What I read so far looks good, though!

internal/configs/named_values.go Show resolved Hide resolved
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

I like this revised design a lot! There was one part inline that I'm a bit confused by. I think we'll need a docs update for this too.

internal/terraform/evaluate.go Show resolved Hide resolved
@jbardin jbardin force-pushed the jbardin/nullable-variable branch from a0a3908 to 3fed9e8 Compare October 29, 2021 21:08
@jbardin jbardin force-pushed the jbardin/nullable-variable branch from 3fed9e8 to 7b7972a Compare October 29, 2021 21:20
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Looks good to me!

internal/terraform/node_module_variable.go Outdated Show resolved Hide resolved
@jbardin
Copy link
Member Author

jbardin commented Nov 1, 2021

I'll follow up with a separate PR for docs

@jbardin jbardin merged commit b91d943 into main Nov 1, 2021
@jbardin jbardin deleted the jbardin/nullable-variable branch November 1, 2021 16:46
@apparentlymart apparentlymart added this to the v1.1.0 milestone Nov 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2021

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

null value is not treated per docs when passed to modules
3 participants