Internal: Code Cleanup

[jtopjian]

This issue will track some internal code cleanup we plan to do over the next while. All PRs related to code cleanup should link to this issue.

At the moment, we plan to:

1. Replace provider-specific validation code with core Terraform helpers (where applicable). Do not add new validation at this time!

2. Rename functions to use the "expand/flatten" pattern seen in other providers (example). More about flatten and expand can be read about here.

3. Attempt to standardize on log and error message formats. Where applicable, use the resource name and the ID of the resource in the message. Example:

   [DEBUG] Retrieved openstack_compute_flavor_v2 <uuid>: ...
   

4. Move non-CRUD functions to a separate. For example, functions in the openstack_dns_zone_v2 resource that are not named *Create, *Read, *Update, or *Delete will be moved to a file called dns_zone_v2.go.

5. Move custom types from types.go into the resource file. For example if there was a custom type used in openstack_dns_zone_v2, move it from types.go to dns_zone_v2.go.

6. Ensure CheckDeleted is used when it's appropriate to do so.

7. Remove d.SetId("") from the delete functions.

8. Check for an error when setting aggregate types.

This list will be updated with any other cleanup items.

A note about naming functions:

Since this provider attempts to support all OpenStack projects, it's important to create names which relate to the specific project and resource in question. This is to prevent case where a Block Storage Key Pair would conflict with a Compute Key Pair (which won't happen - that was just an example). In addition, this provider attempts to support all versions of an OpenStack project. For example, Block Storage v1, v2, and v3.

Functions should be named like so:

• Main resource function: resourceComputeKeypairV2. This defines the *schema.Resource function for the openstack_compute_keypair_v2 function.
• Main data source function: dataSourceComputeKeypairV2.
• CRUD functions: resourceComputeKeypairV2Read and dataSourceComputeKeypairV2Read.
• Expand functions: expandComputeKeypairV2<Field>.
• Flatten functions: flattenComputeKeypairV2<Field>
• All other functions: computeKeypairV2<something something>. Notice how resource and/or dataSource were dropped from the name.

If a function can apply to more than one resource, name the function appropriately using your best judgement.

These changes should not alter the resources' behavior. If we come across refactoring that might alter the behavior, it will be done in a separate issue. If a change introduced a bug, we will fix it and create a new release as soon as possible.

[jtopjian]

@ozerovandrei I've updated this issue with some of the methods I used in the first few PRs. These aren't written in stone, though, so let me know if you want to see something changed.

[ozerovandrei]

@jtopjian rules looks good for now.

I think we will extend them with some additional cases about performing resource updates in several steps.
#258 is a good example of such complex updates.

I checked current PRs and I want to ask - can we also add simple unit tests for all new helpers in the compute_flavor_v2.go, compute_keypair_v2.go and compute_secgroup_v2.go? I think they will be helpful.

[jtopjian]

I checked current PRs and I want to ask - can we also add simple unit tests for all new helpers in the compute_flavor_v2.go, compute_keypair_v2.go and compute_secgroup_v2.go? I think they will be helpful.

Agreed - I think they'll be helpful.

I've added some tests to those three resources where offline unit tests were possible.

[kayrus]

According to this comment we can use validation.StringInSlice for limited input entries. Is a new ticket required for this task?

[kayrus]

Another cleanup case:

remove d.SetId("") at the end of Delete functions.

[jtopjian]

According to this comment we can use validation.StringInSlice for limited input entries. Is a new ticket required for this task?

No, it's listed in the first point at the beginning of this issue:

"Replace provider-specific validation code with core Terraform helpers (where applicable). Do not add new validation at this time!"

However, please do submit changes which span resources. All PRs should work on individual resources.

[jtopjian]

Note to self: before closing this issue, do one final sweep of CheckDeleted in the Read and Delete functions.

[kayrus]

re #592

CheckDeleted also should be added into the LBaaSv2 resources:

• openstack_lb_loadbalancer_v2
• openstack_lb_listener_v2
• openstack_lb_pool_v2
• openstack_lb_member_v2
• openstack_lb_monitor_v2
• openstack_lb_l7policy_v2
• openstack_lb_l7rule_v2

[kayrus]

* openstack_compute_instance_v2.this: Error deleting OpenStack server: Resource not found

In my case the server was removed manually, therefore it is a minor issue.

Do I need to send a PR or will it be covered by yet another cleanup task?

UPD: In addition the admin_pass attribute should be marked as Sensitive: true.

[jtopjian]

I've intentionally been ignoring openstack_compute_instance_v2 until later. openstack_compute_instance_v2 is similar to the port resource in that it's fragile and unique.

If you'd like, please submit a PR with just the addition of CheckDeleted.

With regard to the admin_pass, that's a schema change that wouldn't have been caught in a cleanup. It's a small enough change, so you can bundle it in the same PR.