refactor build interfaces

[bparees]

Attempting to bring some structure to the build interfaces we had scattered about the code... the intent is to define all the interfaces and the "standard" implementation in the util.go file, define the mock impls in the test cases that use them, and reduce the number of interfaces (rather than having an interface per build operation, have an interface that aggregates all the build operations).

this makes it slightly harder to mock the interface since you have to provide more dummy implementations, but it makes it much easier (imho) to understand the set of interfaces involved in the build process and where they're implemented.

[bparees]

@soltysh @mfojtik @csrwng PTAL

i think there is more that can be done here, but again i'd like to get this iteration in to avoid rebase/merge conflict nightmares

[bparees]

[test]

[csrwng]

godoc? (same for interface below)

[bparees]

added

[csrwng]

The builder package seems like an odd place to put the interfaces, maybe they should go in their own client package?

[smarterclayton]

So we don't do this for a reason - to make local testability easier. This is kind of going against the conventions we're using in both Kubernetes and Origin.

[bparees]

having seen what that has wrought, i really disagree with that convention. it has created a nightmare of interfaces that make it really hard to follow what is being implemented, not to mention redundant logic all over the place.

I also don't really see that this reduces local testability, as you can see in the PR i've retained all the existing tests.

[soltysh]

Actually Ben's refactor matches upstream conventions, see https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/controller/replication_controller.go#L46-L51. The only problem I can see here, there's too much create build methods we're using.

[soltysh]

This interface purpose is to provide external services to the controller. I'd propose moving CreateBuildFromConfig and CreateBuildWithImmutableId methods to the controller, because they contain logic that should be part of the controller. The only interaction with other parts of the system I see there is CreateBuild which is part of that interface and all the rest should happen inside controller IMHO.

[soltysh]

Looking more into it, I haven't noticed this interface landed in util.go, sorry. I see a BuildInterface here which should contain create and update methods and the other two methods (CreateBuildFromConfig, CreateBuildWithImmutableId) are just plain util methods, that should not be part of that interface.

[mfojtik]

@soltysh +1 i think the interfaces should be 'clean'

[bparees]

CreateBuildFromConfig and CreateBuildWithImmutableId need to be interfaces somewhere so they can be mocked... I might be inclined to agree that the methods that "just" create the object w/ no other logic should be separated from the more sophisticated paths, though.

[smarterclayton]

I'm not in favor of this change given our existing conventions.

[smarterclayton]

It would be more effective to implement a "real" implementation of ClientWebhookInterface that wraps a client in webhook.

[bparees]

There's no good reason for "ClientWebhookInterface" to exist, all it did was provide CreateBuild methods, there's nothing unique about that that demands the webhook component have its own version of the interface. Which is why i've removed it and replaced it with the more generic BuildClient interface to be used by things that want to create/manipulate Builds and BuildConfigClient interface for things that want to create/manipulate BuildConfigs

[soltysh]

I'm leaning towards @smarterclayton idea of not changing what we have here. What I'd propose is to come up with reasonable naming scheme for case such as here. In there we have 3 different interfaces serving similar purpose, they differ only in the resource type they serve. I'd suggest renaming all of them to <resource>Manager, <resource>Client or whatever else as long as it'll be consistent in this file and among other controllers, my 0.02 💲

[mfojtik]

godoc does not match

[bparees]

i've addressed the non controversial comments (will squash later), will see if i can find a compromise on the interface debate after some discussion with @mfojtik and @soltysh

[bparees]

@smarterclayton let me add my next commit reworking the interface definitions before we continue this discussion, i think it will alleviate at least some of your concerns.

[bparees]

The refactor has been refactored.
Things I suspect @smarterclayton will still not like:

1. There is still a BuildClient and BuildConfigClient aggregate interface, though what they contain has been greatly stripped down. However I did introduce individual interfaces for test mocking, so tests do not need to implement the full Build(Config)Client interface.

2. BuildControllerFactory still has an explicit BuildUpdater field rather than creating a BuildUpdater from the OSClient, as is being done for the "PodManager" and "ImageRepositoryClient". I prefer my approach because (although not currently used for testing) it enabled one to create factories that will create BuildControllers that use the OSClient, but use an alternate implementation of BuildUpdater. I don't really see any meaningful downside to having this be a part of the Factory (I would do it for PodManager and ImageRepositoryClient too, if it were up to me). I also think "ControllerClient" is a pretty unclear concept/term, so that was another motivation to not follow that pattern here.

3. ImageChangeControllerFactory follows the same pattern as Add alpha API documentation #2

4. I moved the Build(Config)Client interfaces into a new package, "client". I think you wanted that part, but you may not like that i left the canonical implementations in that package. I think they make more sense there than anywhere else I might put them (eg putting them in "factory" which is the primary place they're used). util.go is back to what it was. it should probably go away completely, but that's beyond what i want to do here.

5. We also still have a "util" package as well, which should probably go away, but it was not introduced by this refactor.

[smarterclayton]

Again, buildclient.BuilderUpdater would make me feel like you want me to be happy.

[bparees]

i find it helpful when things are clearly spelled out as being interfaces w/o having to dig into them. it tells me immediately that the implementation might be flexible and i need to consider why and how it changes, and tells me i can potentially change it if needed. Having to jump to the definition to determine that is an extra step.

but again, i'll change it, i'm not that hung up on it, but i do think there are multiple valid schools of thought here.

[smarterclayton]

I like this version much better, a few name things and the client stuff left.

[bparees]

Made the final changes in a separate commit, will squash if you're content.

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/898/)

[soltysh]

Was this change intentional?

[bparees]

no, global search/replace error.

[soltysh]

I'd propose renaming the file to clients.go, similar to what we have in pkg/client/client.go.

[bparees]

since there are multiple clients in this file, i think i prefer it as is.

[smarterclayton]

This is more of a convention thing. If you have one file in a package, and you want to call the package a certain something, you generally call the only file in the package the same as the package. That's usually the stronger convention.

[bparees]

so as soon as we add a second file to this package, the name would be ok, right?

[mfojtik]

LGTM 👍

[soltysh]

One small nit, other than that LGTM 🎱

[bparees]

---->squashed<----

[bparees]

@smarterclayton merge at will.

[smarterclayton]

If you have BuildControllerClients, why do you need BuildControllerClient?

[bparees]

because I don't want both and in the interest of your implied pending abstraction of these methods, I didn't think using the method that returned both and then assuming one of the two return values would always be the one i wanted, was the right thing to do.

[bparees]

@smarterclayton removed the BuildControllerClient method.

[smarterclayton]

LGTM [merge]

[openshift-bot]

Evaluated for origin up to 87233e7

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/730/) (Image: devenv-fedora_624)