-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat(security-email): add support for security email #497
feat(security-email): add support for security email #497
Conversation
741fd88
to
941f6d2
Compare
similar to: #495 |
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'm not convinced about the delete behaviour, but other than that it makes sense.
Can you add a test for the resource please?
spacelift/resource_security_email.go
Outdated
} | ||
|
||
func resourceSecurityEmailDelete(ctx context.Context, data *schema.ResourceData, i interface{}) diag.Diagnostics { | ||
return diag.Errorf("deleting security email is 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.
This doesn't seem a great UX. Wouldn't it mean that if you had added this resource, you'd need to remove it from the Terraform state in order to be able to delete the resource?
I wonder if we should do something like this:
- Update the API to allow the security email to be set to an empty string / null; or
- Turn this into a no-op (maybe with a warning).
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'd need to remove it from the Terraform state in order to be able to delete the resource?
yes
I'll turn it into a warning and remove the resource from the state
Signed-off-by: Michal Wasilewski <[email protected]>
bf491c9
to
67aa628
Compare
Signed-off-by: Michal Wasilewski <[email protected]>
Description of the change
related to: https://app.clickup.com/t/8693fq8dx
Adds to the terraform provider support for editing security email.
Note: the graphql api doesn't currently support deleting security email and terraform test sdk runs delete after each test
Type of change
Related issues
Checklists
Development
false
.)go generate
to make sure the docs are up to dateCode review