-
Notifications
You must be signed in to change notification settings - Fork 43
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
Remove panic on imports for TF Plugin Framework #1317
Conversation
@@ -120,7 +120,7 @@ func (*blockSchema) DefaultFunc() shim.SchemaDefaultFunc { | |||
} | |||
|
|||
func (*blockSchema) DefaultValue() (interface{}, error) { | |||
panic("DefaultValue() should not be called during schema generation") | |||
return nil, nil |
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.
Copy the comment here from another location ..
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.
And there's another place I think where we do this.
|
||
func newTestnest() resource.Resource { | ||
return &testnest{} | ||
} | ||
|
||
func (*testnest) schema() rschema.Schema { | ||
return rschema.Schema{ | ||
Attributes: map[string]rschema.Attribute{ |
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.
Adding as requirement for testing import against this resource.
Diff for pulumi-azuread with merge commit d92c3b1 |
Diff for pulumi-random with merge commit d92c3b1 |
3e4089d
to
a968754
Compare
Diff for pulumi-azuread with merge commit def673f |
Diff for pulumi-azuread with merge commit 4328799 |
Diff for pulumi-azuread with merge commit 084e286 |
a9bc479
to
dd56623
Compare
Diff for pulumi-azuread with merge commit ac38249 |
Add a test to prove that resources with blocks no longer panic Co-Authored By: Guinevere Saenger <[email protected]>
3715074
to
b7c2bae
Compare
Diff for pulumi-azuread with merge commit f703fcc |
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.
I don't like that this will swallow a generic error. Can we give fmt.Errorf("default values not supported")
a concrete global value and then only swallow that value (which cannot be user generated).
// internal
var SchemaTimeDefaultErr = fmt.Errorf("default values not supported")
...
// pf/internal/schemashim/attr_schema.go
func (*attrSchema) DefaultValue() (interface{}, error) {
return nil, SchemaTimeDefaultErr
}
...
// pkg/tfbridge/schema.go
func getDefaultValue(tfs shim.Schema, ps *SchemaInfo) interface{} {
if dv, err := tfs.DefaultValue(); dv != nil &&
err != nil &&
!errors.Is(err, SchemaTimeDefaultErr) {
if err != nil {
return nil, err
}
return dv
}
return nil
}
if err != nil { | ||
// Log error output but continue otherwise. | ||
// This avoids a panic on preview such as https://github.com/pulumi/pulumi-cloudflare/issues/460. | ||
glog.V(9).Infof(err.Error()) |
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.
If a user default value function returns an error, I would expect us to halt the pulumi up
and display the error to the user. Swallowing a generic error, especially on v9, seems dangerous.
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.
The issue here is that there’s no fix for the user. We need to implement proper default value handling in the plug-in framework as a followup. Adding this error but emitting it as Info I felt was more informative than simply returning nil.
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.
Nope, we can't halt pulumi up in this case! This is the entire point of this PR. The code path is used to try to make import generated code better by taking default values into account, but Plugin Framework does not at this point support detecting default values, so the code must proceed with best-effort like returning nil. I think the behavior as written is what we want (modulo code style).
pkg/tfbridge/schema.go
Outdated
@@ -1407,7 +1406,12 @@ func extractInputs(oldInput, newState resource.PropertyValue, tfs shim.Schema, p | |||
} | |||
|
|||
func getDefaultValue(tfs shim.Schema, ps *SchemaInfo) interface{} { | |||
if dv, _ := tfs.DefaultValue(); dv != nil { | |||
if dv, err := tfs.DefaultValue(); dv != nil { |
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.
We have changed DefaultValue
to return nil, fmt.Errorf("default values not supported")
. The error won't even be logged here, since dv == nil
. We need to also check that err != nil
.
…t nil or SchemaDefaultValueErr
Using a custom error is the first thing I tried; the issue is that we can't declare it inside We've been swallowing an error on tfs.DefaultValue for quite a while. I'm somewhat concerned that explicitly returning an error here is going to have other effects I'm not aware of. |
Diff for pulumi-azuread with merge commit bb64958 |
Diff for pulumi-random with merge commit bb64958 |
// SchemaDefaultValueErr catches the situation where pf/schemashim.DefaultValue() is called by ExtractInputsFromOutputs. | ||
// To avoid a panic, we return this error and log it to Info. | ||
// See https://github.com/pulumi/pulumi-cloudflare/issues/460 | ||
var SchemaDefaultValueErr = fmt.Errorf("default values not supported") |
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.
You can just say Internal
also in the comment - not something we expect users to need to link to.
I think the is good to go modulo lint issues.
|
@guineveresaenger can you reopen a PR in your name since you're driving this work. Then I can approve. thanks. |
This PR removes the panics on
getDefaultValue
on the Type, Attr, and Block schemas in the TF Plugin Framework schemashim, as they are causing issues when importing resources using the Plugin Framework.Instead, we emit nil and an error, which we log using our standard logger, allowing the code to run without panic. I have build and run this against the user reported panic and verified that this resolves the issue.
We will file a followup issue to implement properly handling default values directly inside the
pf
package.Fixes pulumi/pulumi-cloudflare#460.