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

fix: Refresh and destroy on resources with an out of band deletion #154

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

glena
Copy link
Contributor

@glena glena commented Aug 1, 2023

DeploymentSettings, Team and Webhook api client now returns nil on 404 responses so the provider treats it accordingly and recognize them as deleted resources

Fix #146

image image

@glena glena force-pushed the german/fix-deleted-deployment branch from 72eaf83 to c3d27ac Compare August 1, 2023 16:22
@glena glena requested review from pgavlin and blampe August 1, 2023 16:29
Comment on lines 533 to 539
statusCode := pulumiapi.GetErrorStatusCode(err)
if statusCode == http.StatusNotFound {
// deleteResponse causes the resource to be deleted from the state.
var deleteResponse = &pulumirpc.ReadResponse{Id: "", Properties: nil}
// If it's a 404 error, this resource was probably deleted.
return deleteResponse, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes the problem for DeploymentSettings, but it occurs to me that all of our other resources probably have the same bug. Can we do this check one layer up in the provider instead of the resource? I think this would cover it:

https://github.com/pulumi/pulumi-pulumiservice/blob/main/provider/pkg/provider/provider.go#L205-L209

Aside: several resources don't implement Read (ugh) but we don't need to fix that here. It would be so nice to some day generate this provider from an OpenAPI spec!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look tomorrow morning about that, thx 🙇

Copy link
Contributor Author

@glena glena Aug 3, 2023

Choose a reason for hiding this comment

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

I just found teams is handling the "not found" in the way I mention yesterday to you, returning nil instead of propagating the error, I will change to handle it in the same way https://github.com/pulumi/pulumi-pulumiservice/blob/main/provider/pkg/provider/team.go#L192-L194 (unless the api is returning a 404 too and it is just returning an error? https://github.com/pulumi/pulumi-pulumiservice/blob/master/provider/pkg/internal/pulumiapi/teams.go#L108-L112)

For webhooks, it is weird, I guess it is returning a 404 too? (otherwise it accounts as it always exists as it is returning the req.id in the response, no?) I will ping you to see how can I actually test it (I dont want to use pulumi org and my personal one does not have it enabled https://github.com/pulumi/pulumi-pulumiservice/blob/main/provider/pkg/provider/webhook.go#L374-L419)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed, teams and webhooks suffer from the same thing
image

image

I will add those 2 tomorrow morning and ask you again for review, thx for bringing it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image image

@glena glena changed the title fix: Refresh and destroy on DeploymentSettings with an out of band deletion fix: Refresh and destroy on resources with an out of band deletion Aug 4, 2023
@glena glena force-pushed the german/fix-deleted-deployment branch from 3d939e3 to 58096ef Compare August 4, 2023 14:41
Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Amazing!

@glena glena merged commit 7450e23 into main Aug 4, 2023
12 checks passed
@glena glena deleted the german/fix-deleted-deployment branch August 4, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh and destroy on DeploymentSettings fails with 404 if there is an out of band deletion
3 participants