-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Catch 404s when deleting default networks. #2117
Catch 404s when deleting default networks. #2117
Conversation
@@ -263,7 +263,11 @@ func resourceGoogleProjectCreate(d *schema.ResourceData, meta interface{}) error | |||
} | |||
|
|||
if err = forceDeleteComputeNetwork(project.ProjectId, "default", config); err != nil { | |||
return fmt.Errorf("Error deleting default network in project %s: %s", project.ProjectId, err) | |||
if isGoogleApiErrorWithCode(err, 404) { |
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.
Can't we instead query for the existence of a default network and skip the call to delete if it already doesn't exist?
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 could! I chose this way for three reasons:
- Either way we'd end up with a network call, except in the most common case we'd end up with two if we checked for existence first
- I'm not sure how the current organizational policy works (whether a network is created, then deleted, or if it's just not created at all) or if any future improvements may lead to a network being created then deleting, and I worried about race conditions
- Returning a 404 when trying to delete something never seems like an appropriate response, as far as I can think?
Because of that, I couldn't see any benefit to it. Is there a reason you think it'd be better?
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 was just thinking about the error on the customer logs related to the delete of the inexistent network, but I'm not strongly opinionated either way.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
As seen in hashicorp/terraform-provider-google#3582, it is now possible to set an organization policy that removes the default network from a project when it's created. This means it's now possible that Terraform's attempt to delete that default network will encounter an error saying the network is not found. Because what Terraform wanted was achieved, even if not by Terraform, we shouldn't raise that error, we should ignore it.
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
c2bccb9
to
dc43084
Compare
As seen in hashicorp/terraform-provider-google#3582, it is now
possible to set an organization policy that removes the default network
from a project when it's created. This means it's now possible that
Terraform's attempt to delete that default network will encounter an
error saying the network is not found. Because what Terraform wanted was
achieved, even if not by Terraform, we shouldn't raise that error, we
should ignore it.
Release Note for Downstream PRs (will be copied)