Break apart the Kubernetes Client #214
Conversation
|
Testing: Run the normal broker tests. |
rthallisey
force-pushed
the
breakup-clients
branch
from
91bab7d
to
13ecb88
Compare
Make a seperate directory for clients so they are not apart of the APB code. This will make the clients easy to initialize and create new ones.
rthallisey
force-pushed
the
breakup-clients
branch
from
13ecb88
to
21991d0
Compare
There was a problem hiding this comment.
I actually think I captured most of my thoughts for this in the #222 review. Main thing I would add is moving more of the client stuff out of apb/client.go somewhere here:
I think we should push any remaining client code unrelated to actually running an apb into the clients package, then we should reconsider renaming apb/client.go to something that more accurately describes what it's doing. Semantically, it's the part of the apb package that's responsible for actually executing apb methods. Could almost be apb/apb.go? Curious what you guys think.
| @@ -11,9 +11,8 @@ import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
| restclient "k8s.io/client-go/rest" | |||
|
|
|||
| "github.com/openshift/ansible-service-broker/pkg/clients" | |||
There was a problem hiding this comment.
Wow I never realized how ugly our import is :( not really a problem with this PR, just something I noticed finally. Would be cool if we could get: openshift.io/asb/
There was a problem hiding this comment.
On the plus side importing our a clients package is good.
| } | ||
|
|
||
| clientset, err := clientset.NewForConfig(clientConfig) | ||
| k8s, err := clients.Kubernetes(log) |
There was a problem hiding this comment.
I like the simplicity of this. I do not like how the Client struct has a bunch of clients in it. Will keep looking at this and other PRs to see if I understand the need for it. I know originally it was
type Client struct {
dockerClient ...
}
| return config, nil | ||
| } | ||
|
|
||
| func Kubernetes(log *logging.Logger) (*clientset.Clientset, error) { |
There was a problem hiding this comment.
This is definitely cleaner. 👍
|
I'm going to approve this on my side. The main concern is similar to @eriknelson need to remove Clients struct. But the changes here seem reasonable. |
|
@jmrodri Plan is for me to follow up this series with a PR that removes the Client, which is reasonable because this is a good step forwards and looks stable. |
There was a problem hiding this comment.
Sorry, @shawn-hurley brought up a good point on the other PR. I think anything exported needs docstrings to help him out with linter stuff.
Also, do we want to close this and the docker PR so everything is centralized on #222 @rthallisey @jmrodri ?
|
Working in #222 |
Make a separate directory for clients so they are not part of the APB code. This will make the clients
easy to initialize, a single place to create the object, and adds the ability to easily create new ones.
Partially-fixes: #213