-
Notifications
You must be signed in to change notification settings - Fork 118
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
Replace usage of deprecated fields in schema.Resource #231
Conversation
…gs func in resource_id (#230)
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.
A suggestion, but otherwise it looks good.
Delete: schema.RemoveFromState, | ||
CreateContext: CreateInteger, | ||
ReadContext: schema.NoopContext, | ||
DeleteContext: DeleteInteger, |
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.
It's annoying that there is no schema.RemoveFromStateContext
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.
Would it make sense to implement the DeleteContextFunc
once in a shared file and then have each resource use that instead to have to implement the dedicated DeleteX
methods?
Not super important, but I thought I'd suggest.
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.
@detro your suggestion makes sense to me. Have created a shared DeleteContext
func and added it to provider.go
.
internal/provider/resource_pet.go
Outdated
Delete: schema.RemoveFromState, | ||
CreateContext: CreatePet, | ||
ReadContext: schema.NoopContext, | ||
DeleteContext: DeletePet, |
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.
as per above
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.
Done (see above).
internal/provider/resource.go
Outdated
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" | ||
) | ||
|
||
func DeleteContext(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { |
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.
Nitpick: would it make sense to use a name for this that doesn't clash with the field it's used in?
Reading below:
{
// ...
DeleteContext: DeleteContext,
// ...
}
it's a bit ... odd.
Admittedly, coming up with a name can be hard (as usually, naming things always is).
How about removeResourceFromState()
, since that's what d.SetId("")
is for?
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.
Fair enough. Done.
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. |
Closes #230.