Copilot commented on code in PR #331:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/331#discussion_r3879795007
##########
cloudstack/resource_cloudstack_ipaddress.go:
##########
@@ -276,23 +276,21 @@ func resourceCloudStackIPAddressRead(d
*schema.ResourceData, meta interface{}) e
}
func resourceCloudStackIPAddressDelete(d *schema.ResourceData, meta
interface{}) error {
- if !d.Get("is_source_nat").(bool) {
- cs := meta.(*cloudstack.CloudStackClient)
-
- // Create a new parameter struct
- p := cs.Address.NewDisassociateIpAddressParams(d.Id())
-
- // Disassociate the IP address
- if _, err := cs.Address.DisassociateIpAddress(p); err != nil {
- // This is a very poor way to be told the ID does no
longer exist :(
- if strings.Contains(err.Error(), fmt.Sprintf(
- "Invalid parameter id value=%s due to incorrect
long value format, "+
- "or entity does not exist", d.Id())) {
- return nil
- }
+ cs := meta.(*cloudstack.CloudStackClient)
- return fmt.Errorf("Error disassociating IP address %s:
%s", d.Id(), err)
+ // Create a new parameter struct
+ p := cs.Address.NewDisassociateIpAddressParams(d.Id())
+
+ // Disassociate the IP address
+ if _, err := cs.Address.DisassociateIpAddress(p); err != nil {
+ // This is a very poor way to be told the ID does no longer
exist :(
+ if strings.Contains(err.Error(), fmt.Sprintf(
+ "Invalid parameter id value=%s due to incorrect long
value format, "+
+ "or entity does not exist", d.Id())) {
+ return nil
}
+
+ return fmt.Errorf("Error disassociating IP address %s: %s",
d.Id(), err)
Review Comment:
`fmt.Errorf` should wrap the original error using `%w` instead of formatting
it with `%s`, so callers can reliably unwrap/inspect the underlying CloudStack
error (and so error chains are preserved in logs and diagnostics). Suggested
change: use `%w` for `err`.
##########
cloudstack/resource_cloudstack_ipaddress.go:
##########
@@ -276,23 +276,21 @@ func resourceCloudStackIPAddressRead(d
*schema.ResourceData, meta interface{}) e
}
func resourceCloudStackIPAddressDelete(d *schema.ResourceData, meta
interface{}) error {
- if !d.Get("is_source_nat").(bool) {
- cs := meta.(*cloudstack.CloudStackClient)
-
- // Create a new parameter struct
- p := cs.Address.NewDisassociateIpAddressParams(d.Id())
-
- // Disassociate the IP address
- if _, err := cs.Address.DisassociateIpAddress(p); err != nil {
- // This is a very poor way to be told the ID does no
longer exist :(
- if strings.Contains(err.Error(), fmt.Sprintf(
- "Invalid parameter id value=%s due to incorrect
long value format, "+
- "or entity does not exist", d.Id())) {
- return nil
- }
+ cs := meta.(*cloudstack.CloudStackClient)
- return fmt.Errorf("Error disassociating IP address %s:
%s", d.Id(), err)
+ // Create a new parameter struct
+ p := cs.Address.NewDisassociateIpAddressParams(d.Id())
+
+ // Disassociate the IP address
+ if _, err := cs.Address.DisassociateIpAddress(p); err != nil {
+ // This is a very poor way to be told the ID does no longer
exist :(
+ if strings.Contains(err.Error(), fmt.Sprintf(
+ "Invalid parameter id value=%s due to incorrect long
value format, "+
+ "or entity does not exist", d.Id())) {
+ return nil
}
Review Comment:
The PR description says “propagate whatever CloudStack returns instead of
swallowing it”, but this delete path still swallows (returns `nil` for) the
“entity does not exist” error. If this behavior is intentional (and it likely
is for idempotent deletes), the PR description should be adjusted to reflect
that the “already gone” case remains a special-case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
