terraform: export more methods, deduplicate client funcs

[kayrus]

@jtopjian this PR allows external openstack based provider to use config methods + removed duplicate code for simple openstack service client.

[jtopjian]

@kayrus Thanks!

@ozerovandrei I'm out of touch with the Terraform Provider. At your convenience, do you mind looking this over?

[kayrus]

FYI: this PR doesn't change anything on the Terraform Provider side. It just adds more flexibility for other custom providers.

[jtopjian]

Ah, I understand.

In general, the changes look okay to me. The only concern I have is around using a function as a parameter. I realize this is legal in Go, but I find the style to be "clever" and not immediately understandable to the reader.

[kayrus]

@jtopjian

I realize this is legal in Go, but I find the style to be "clever" and not immediately understandable to the reader.

I thought about using the gophercloud.EndpointOpts + *gophercloud.ServiceClient as additional parameters, but this would again explode the code :\

[jtopjian]

hm... I am fine with making the methods exported, but there's something nagging me about the function parameter that I can't put my finger on. Unfortunately, I have not had enough time to sit down and see if I can work it out. It's not personal, there's just something about it that has caught my attention and I want to sort it out.

Side note: while quickly scrolling through the code, I see that this PR should be rebased on master due to #122.

[kayrus]

@jtopjian it is already rebased.

[jtopjian]

Ah, I see. The MessagingV2Client method wasn't forgotten, it's that the NewMessagingV2 function signature is different than the others and therefore can't take advantage of the function type created.

Doesn't this create inconsistency in how the file is laid out? Before this PR, each client method was autonomous, even if code was duplicated. But now some client methods call a shared method while other client methods are handled in a custom way. In other words, while I understand if part of the purpose of this PR is to deduplicate code, is that code being deduplicated just for the sake of deduplication? Is this an issue of premature deduplication so to speak?

[kayrus]

My logic was to export two more methods and simplify code for common openstack clients. This PR significantly reduces the amount of code (for example see the amount of code here: https://github.com/sapcc/terraform-provider-ccloud/blob/ab841909217f65e88f9fd74b7361e6ca74dc1d97/ccloud/config.go#L294..L409), and allows to reuse the Authenticate method for non-common clients.

Take a look on the patch, which I'm going to apply to our custom provider: https://gist.github.com/kayrus/319e99b882d323fed9c64d03c608e239

[jtopjian]

Right - exporting some methods so that other code can take advantage of it is fine with me. The two areas that I'm questioning are the deduplication of code from some client creation methods and the typed function as a result of that.

The majority of the client creation methods appear to be similar and could be reduced, yes. But is it a good idea?

Pros of deduplicating the client creation methods:

1. Reduced code.
2. If a common change needs to happen, it only has to be done in the central ServiceClient method.

Cons of deduplicating the client creation methods:

1. Not all client creation methods can be deduplicated.
2. It introduces a function type that not all service clients can adhere to.

Again, I haven't had time to sit down and look at this. I'm only going off of diffs which is not ideal.

On a side note: It's good to know you folks are still using Lime. I came across it a long time ago and considered trying it internally at $work since we have similar needs. 🙂

[kayrus]

@jtopjian ok, take your time and let me know your decision.

[jtopjian]

@kayrus Thanks - sorry for the delay.

I realize this may be a difference of opinion about code design. Whether having the same set of boilerplate code in all client creation methods or abstracting some into a common method, the result is still the same. In that way, my personal design preference is for the former. To me, having a "flat" set of client creation methods is easier to comprehend than adding a layer of abstraction for some of them. But again, I realize this may just be design preference.

This would be altogether different if the client creation functions all followed the same function signature. Because they don't lends to support that it is not possible to abstract all client creation functions. And even when the function signature matches, the body of the method itself may differ from the others. I realize that each of these examples only have one client supporting the example (messaging and objectstorage). But they are deviations nonetheless and should be accounted for.

This, however, is extremely difficult. I can't think of an easy way, even if changes were made to the core of Gophercloud, to have a standard function signature and method body for all service client creations here.

With that in mind, because this is just for the Terraform authentication, if you and @ozerovandrei are okay with this design, then so am I. If this was more general Gophercloud code, I'd want to, somehow, find a way to ensure all client creation functions could conform to the same type.

And with that in mind, perhaps the common function type and the common method coud be named a little differently to better convey what they are:

Instead of newClientFunc, how about commonServiceClientFunc? And instead of ServiceClient how about CommonServiceClientInit?

This may better describe that these only handle "common" service clients and not all service clients. As well, Init may convey that some additional initialization is taking place, too.

Thoughts?

[kayrus]

@jtopjian

But again, I realize this may just be design preference.

I see a maintenance advantage. It is more easy to add a code change in one place, the probability of copy-paste or human error decreases.

With that in mind, because this is just for the Terraform authentication

+1

@ozerovandrei objections?

[kayrus]

@jtopjian the PR is ready. Let me know if you have any objections.