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

functions: Fix panics in defaults #27979

Merged
merged 2 commits into from
Mar 5, 2021
Merged

functions: Fix panics in defaults #27979

merged 2 commits into from
Mar 5, 2021

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Mar 4, 2021

Two commits, addressing the two separate issues in #27385:

functions: Fix defaults null collections panic

When applying default values to collection types, null collections in the input should result in empty collections in the output.

Fixes #27385 (original report)

functions: Fix defaults mismatched types fallback

We allow primitive fallback values which have mismatched types, but only if there is a conversion to the target type. Previously we would allow unsafe conversions (e.g. string to bool), but later had no capacity to return an error if the conversion failed due to the value of the fallback being unable to convert to the target type.

This commit makes the more conservative requirement that default fallback values must have a safe conversion.

Fixes #27385 (follow-up reports)

When applying default values to collection types, null collections in
the input should result in empty collections in the output.
We allow primitive fallback values which have mismatched types, but only
if there is a conversion to the target type. Previously we would allow
unsafe conversions (e.g. string to bool), but later had no capacity to
return an error if the conversion failed due to the value of the
fallback being unable to convert to the target type.

This commit makes the more conservative requirement that default
fallback values must have a safe conversion.
@alisdair alisdair added functions 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Mar 4, 2021
@alisdair alisdair requested a review from a team March 4, 2021 15:47
@alisdair alisdair self-assigned this Mar 4, 2021
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #27979 (66f8d1c) into main (e9c7f37) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
lang/funcs/defaults.go 83.76% <100.00%> (+6.36%) ⬆️
terraform/node_resource_plan.go 96.11% <0.00%> (-1.95%) ⬇️
terraform/evaluate.go 52.47% <0.00%> (-0.42%) ⬇️

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Yay, very nice! Thank you for digging into this ❤️

@alisdair alisdair merged commit 98899df into main Mar 5, 2021
@alisdair alisdair deleted the alisdair/defaults-fixes branch March 5, 2021 19:39
@ghost
Copy link

ghost commented Apr 5, 2021

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 as resolved and limited conversation to collaborators Apr 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged functions
Projects
None yet
2 participants