🏃 Refactor: use Service struct field more

[hidekazuna]

What this PR does / why we need it:

This PR refactored to use gophercloud clients and logger in Service struct instead of function argument.
We can easily log something using s.logger without adding logger to function argument.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• if necessary:
  • includes documentation
  • adds unit tests

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [hidekazuna]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

@hidekazuna Can you elaborate a bit what the advantages are of this approach? In the cluster-api repo there's a similar problem and it's usually solved by handing over "input" structs with the required fields, e.g.: https://github.com/kubernetes-sigs/cluster-api/blob/master/test/framework/machinedeployment_helpers.go#L38-L43

I have to say I don't have a well thought-out opinion yet about the advantages/disadvantages of using "Input" structs versus putting more into structs and using methods. But right now I think I like Input structs more. Let's discuss it a bit what we want. Because after we decide that we will probably refactor a lot of code in this direction over time. (so please let's try to figure that out first before moving on with this PR and other similar refactorings)

[sbueringer]

@hidekazuna Can you elaborate a bit what the advantages are of this approach? In the cluster-api repo there's a similar problem and it's usually solved by handing over "input" structs with the required fields, e.g.: https://github.com/kubernetes-sigs/cluster-api/blob/master/test/framework/machinedeployment_helpers.go#L38-L43

I have to say I don't have a well thought-out opinion yet about the advantages/disadvantages of using "Input" structs versus putting more into structs and using methods. But right now I think I like Input structs more. Let's discuss it a bit what we want. Because after we decide that we will probably refactor a lot of code in this direction over time.

/cc @vincepri or @fabriziopandini If you have some time, it would be nice if you can provide some context / reasoning behind "Input structs" vs "more fields in structs + using methods instead of funcs"

[hidekazuna]

@sbueringer By using method and struct field which is used almost functions like gophercloud client, Logger, and clusterName, we can more focus on method specific matter, and easy to read.

By the way, as you know, CAPA and CAPZ have scope.

AWS: https://github.com/kubernetes-sigs/cluster-api-provider-aws/tree/main/pkg/cloud/scope
CAPZ: https://github.com/kubernetes-sigs/cluster-api-provider-aws/tree/main/pkg/cloud/scope

Their scope has not only client and logger but also xxCluster or XXMachine etc. Scope seems wider than our Service struct. We would need more time to create scope, so only take scope idea.

[fabriziopandini]

I can give some context about the CAPI E2E test framework helpers.

The current approach of using input struct derives from the examples in https://onsi.github.io/ginkgo/#global-shared-behaviors and basically supports the idea that a func with many input parameter is hard to read; using input parameters struct the readability improves because each input values is "named" by using the fields in the input struct.

It is also important to notice that funcs in the CAPI E2E test framework helper are basically "standalone", because there is no notion of state that is shared across different function calls.

[sbueringer]

@hidekazuna
I agree with the current PR.
/lgtm

I don't agree with the idea mentioned here: #870 (comment) to put things like cluster name into the struct just because it's convenient.
I don't see how a cluster name makes sense in our current service abstractions as the services abstract away OpenStack services.

[hidekazuna]

I don't agree with the idea mentioned here: #870 (comment) to put things like cluster name into the struct just because it's convenient.
I don't see how a cluster name makes sense in our current service abstractions as the services abstract away OpenStack services.

@sbueringer I understand putting clusterName into struct is not fit for current Service struct purpose: abstract layer using OpenStack services, thanks.

[hidekazuna]

/hold cancel