-
Notifications
You must be signed in to change notification settings - Fork 2
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: handle resources deleted outside TF provider #97
Conversation
@@ -142,6 +142,12 @@ func (r *ResourceResourceClass) Read(ctx context.Context, req resource.ReadReque | |||
return | |||
} | |||
|
|||
if httpResp.StatusCode() == 404 { | |||
resp.Diagnostics.AddWarning("Resource class not found", fmt.Sprintf("The resource class (%s) was deleted outside Terraform", data.ID.ValueString())) |
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: I wonder if we should rather just say "The resource class (%s) does not exist"?
How do we know the resource actually ever existed? espescially if someone is using terraform state importing to add a resource they created by hand?
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.
How do we know the resource actually ever existed?
The case here is that the resource is in the TF state but we can't read it because we don't find it anymore.
I think it would be helpful to see this message in that case, no?
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.
Generally looks good - just one thought on the language of the messages
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.
Nice work 🚀
@@ -46,6 +46,10 @@ func TestAccResourceApplicationDeletedOutManually(t *testing.T) { | |||
|
|||
orgID := os.Getenv("HUMANITEC_ORG") | |||
token := os.Getenv("HUMANITEC_TOKEN") | |||
APIHost := os.Getenv("HUMANITEC_HOST") |
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: variables should be lowercased
APIHost := os.Getenv("HUMANITEC_HOST") | |
apiHost := os.Getenv("HUMANITEC_HOST") |
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 are right, fixed it!
207ee74
to
1726d08
Compare
This makes the provider able to handle deletion of resources outside TF.
Integration tests have been added where they were missing.