resources: WAF: handles properly when the target has been deleted on Cloudflare's side

[XaF]

In the previous situation, when the target had been deleted, the Read
accion would fail and return an error. However, this would be expected
that when this happen, the object should be considered as not existing
anymore and the process should continue. This thus fixes that and makes
sure that we avoid erroring out when a WAF Rule, Group or Package has
been deleted on Cloudflare's side.

[jacobbednarz]

Overall I like the simplicity of this error checking and handling however I wonder if we could be more idiomatic and use the error interface type to have this hang of the error instead of a generic method?

const (
    CLOUDFLARE_INVALID_OR_REMOVED_WAF_PACKAGE_ERROR_ID = 1002
    CLOUDFLARE_INVALID_OR_REMOVED_WAF_RULE_ERROR_ID = 1003
)

func (r *Error) IsCloudflareWAFErrorCode() bool {
    // code from `cloudflareErrorIsOneOfCodes` can go here referencing the constants from above
}

// ... example code in file
group, err := client.WAFGroup(zoneID, packageID, groupID)
if err != nil {
  if err.IsCloudflareWAFErrorCode() {
    d.SetId("")
    return nil
  }

  return err
}

What do you think?

[XaF]

I thought about doing it specifically for WAF (in general) at the beginning but:

• The API documentation does not list those errors, so nothing guarantees that for 1003 error for
  WAF groups couldn't be used for something else in WAF rules, since when reaching a WAF rule through the API we do not consider the group (are we sure error codes are not gonna be reused for another endpoint at the moment ?)
• The way we do things, if made too specific, cannot be reused for anything else... while the way to handle errors and read error codes could be the same for any kind of object (for now, we just use that for WAF, but we might want to reuse it for other things?)

However, it's true that I would agree that using the error interface type could be nicer. So maybe I could:

• rename the method into IsCloudflareErrorOneOfCodes (without WAF) and keep a codes []int parameter
• create a IsWAFRuleNotFound method on the error interface type for the waf rule resource, that calls the IsCloudflareErrorOneOfCodes method with the right parameter
• create a IsWAFGroupNotFound method on the error interface type for the waf group resource
• create a IsWAFPackageNotFound method on the error interface type for the waf package resource
• Call those methods the way you suggested above

This would take advantage of cleaner method names, while not creating a too general approach, that can thus easily be reused for other resources in the future if needed. Thoughts?

[jacobbednarz]

I think that's a reasonable compromise and gets us into a better place, +1 to seeing it in action. Thanks!

[XaF]

Will make the changes tomorrow!

[XaF]

Apparently, and that seems to have been my original reason for doing a function taking parameters instead of the error interface (I remember trying it and then going the other way), but it's seems it is not possible to do: invalid receiver type *error (error is an interface type).

Would we want to implement that on the error subtype, hence requiring casting when making the call? Feels to me it might be less readable if we chose to do so? :(
Thoughts?

[XaF]

Should I put those into their own file or is that fine to have per-file constants here ?

[jacobbednarz]

Happy to start here and we can always extract them out and DRY them up if needed.

[jacobbednarz]

@XaF I think this in a better place; I wouldn't want better to get in the way of perfect here. We can always explore the error type in another PR if we think it's warranted.

[XaF]

Perfect then!
Do you need me to squash the commits and push force ?

[jacobbednarz]

All good! Just waiting on integration to complete and I'll squash and merge.