-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Return errors on parse failures for config.Try*
#9407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHANGELOG_PENDING needs to be cleaned up; LGTM otherwise.
sdk/go/pulumi/config/require.go
Outdated
"github.com/pulumi/pulumi/sdk/v3/go/pulumi" | ||
) | ||
|
||
func failf(format string, a ...interface{}) { | ||
panic(fmt.Sprintf(format, a...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd panic with an error here to make it a bit easier to recover if necessary.
panic(fmt.Sprintf(format, a...)) | |
panic(fmt.Errorf(format, a...)) |
Diff for pulumi-azuread with merge commit 2ce4e97 |
Diff for pulumi-random with merge commit 2ce4e97 |
Diff for pulumi-kubernetes with merge commit 2ce4e97 |
Diff for pulumi-gcp with merge commit 2ce4e97 |
Diff for pulumi-azure with merge commit 2ce4e97 |
Diff for pulumi-aws with merge commit 2ce4e97 |
Diff for pulumi-random with merge commit a14052d |
Diff for pulumi-azuread with merge commit a14052d |
Diff for pulumi-kubernetes with merge commit a14052d |
Diff for pulumi-gcp with merge commit a14052d |
Diff for pulumi-azuread with merge commit b3ada87 |
Diff for pulumi-random with merge commit b3ada87 |
Diff for pulumi-kubernetes with merge commit b3ada87 |
Diff for pulumi-azure with merge commit a14052d |
Diff for pulumi-aws with merge commit a14052d |
Diff for pulumi-gcp with merge commit b3ada87 |
Diff for pulumi-azure with merge commit b3ada87 |
Description
config.Try*
returns an error message if a config key is missing, but there is currently no way to guarantee that a key is pulled from the config. When a non-string primitive key is tried, a failure to parse leads returns the type appropriate zero value. This is super counterintuitive to me. Especially sinceconfig.TryObject
does return a parse error when appropriate.This is a breaking change, but it is hard to imagine a situation where
config.Try*
is used correctly. Therefore I believe the change is appropriate.This PR changes the behavior of
config.Try*
to return an error if either the requested key is missing in the config or if the key is invalid for the requested type.Example
PS: We do the same thing for
config.Require*
, and I made the same change.Checklist