Add err checks on aggregate on fw_v2 resources #1592
Conversation
Not really... BTW, There is a special |
| d.Set("status", group.Status) | ||
| d.Set("region", GetRegion(d, config)) | ||
|
|
||
| if err := d.Set("ports", group.Ports); err != nil { | ||
| diag.Errorf("Unable to set ports for openstack_fw_group_v2 %s: %s", group.ID, err) |
There was a problem hiding this comment.
shouldn't we return the diag.Errorf? And does it make sense to check all Set err codes?
There was a problem hiding this comment.
shouldn't we return the diag.Errorf?
yes
And does it make sense to check all Set err codes?
Only the aggregate types (schema.TypeList, schema.TypeSet, and schema.TypeMap)
but we probably want to standardize this. We are logging it in various resourcess, but not in all
There was a problem hiding this comment.
There is also a change to set Bool to String and String to Bool ;)
There was a problem hiding this comment.
I set TF_SCHEMA_PANIC_ON_ERROR=1 on all ci in #1593 and everything passed. From various docs i found that it was defaulted to 1 when TF_ACC is set on sdkv2. So probably we were able to catch all the possible issues on the ci.
I'm not sure whether we want to implement this tbh. We have it in some resources but not all, and it is just logged. I came across it while going through the docs and I recalled the checks werent added of fw resources.
I think i'll close the PR as it is a more "generic" issue, to standardize the check.
I wonder what could be a good way of standardizing this. I checked a bit the aws provider and they only check for attributes they flatten etc. Checking all aggregate types + Bool + String seems quite an overkill to me.
Not sure how you feel but the aws approach seems good to me. Checks the attributes with the highest possibility of being wrong, but doesn't add 100s of checks on all kind of attributes.
I'm reading that this is enabled by default on sdkv2. But i'll trigger a ci pipeline to see what we get |
Add error checking on aggregate types for fw_v2 resources based on best practises