Ensure initial state set when job started. Reduce repition in job definitions. Allow jobs to be mocked out at broker level #909
Conversation
openshift-ci-robot
added
the
size/XL
|
I really like the changes to the engine here. This to me is the next logical step of @eriknelson works to simplify the creation of jobs. I think we need to hold this until we branch for the release. |
shawn-hurley
added
the
do-not-merge/hold
rthallisey
added
3.10 | release-1.2
openshift-bot
added
the
needs-rebase
maleck13
force-pushed
the
work-engine-job-improvements
branch
from
72f76e5
to
c23ef74
Compare
openshift-bot
removed
the
needs-rebase
maleck13
force-pushed
the
work-engine-job-improvements
branch
from
c23ef74
to
7fc1751
Compare
There was a problem hiding this comment.
I'm not 100% sure of the need for the WorkFactory. Feels like we went from concrete structs to methods that "look like structs'.
If we do plan on keeping the WorkFactory I would prefer that it have New*Job style methods instead.
NewProvisionJob(...)
NewBindJob(...)
etc.
If would be nice if it could've been NewWork(method string, ...) but because the parameters are so diverse so just renaming them to New*Job would be nicer IMO. I don't like ProvisionJob(...) because it looks like a struct initialization but with incorrect parens. To avoid confusion, I prefer the New prefix.
pkg/app/app.go
Outdated
| @@ -282,8 +282,11 @@ func CreateApp(args Args, regs []registries.Registry) App { | |||
| KeepNamespaceOnError: app.config.GetBool("openshift.keep_namespace_on_error"), | |||
| } | |||
| apb.InitializeClusterConfig(clusterConfig) | |||
| brokerNS := app.config.GetString("openshift.namespace") | |||
| // initialize the work factory | |||
| workFactory := broker.NewWorkFactory() | |||
There was a problem hiding this comment.
I'm not sure of the need for the WorkFactory.
There was a problem hiding this comment.
The intention here was creating an interface that would encapsulate the creation of work and allow a mock to be used later down the line to do tests against a running broker without it needing to actually start pods etc. Initializing the stuct directly in the code makes it difficult to control the behavior. Really just saw it as a step in the direction of a more unit testable broker.
Is the concern it is premature abstraction? Can change to use the struct if preferred and can always add the interface back at a later stage. The key is probably the dependency injection of in line initialization.
pkg/broker/jobs.go
Outdated
| func (j *apbJob) Method() apb.JobMethod { | ||
| return j.method | ||
| } | ||
|
|
There was a problem hiding this comment.
ID and Method are good additions.
pkg/broker/jobs.go
Outdated
| run: func(exec apb.Executor) <-chan apb.StatusMessage { | ||
| return exec.Provision(j.serviceInstance) | ||
| // ProvisionJob will setup a Work implementation that will perform the provision work | ||
| func (wf *workFactory) ProvisionJob(si *apb.ServiceInstance) Work { |
There was a problem hiding this comment.
I'm not a fan of using the old struct name as a function now. It feels confusing to see something that looks like a "struct" vs something that is a method. For example when I see ProvisionJob I'd expect to see {} after it like: ProvisionJob{id: "foo", blah blah blah}.
If we're going to keep these I prefer making them look like constructor methods: NewProvisionJob, NewBindJob, etc.
There was a problem hiding this comment.
Sure happy to change these
| return nil | ||
| } | ||
|
|
||
| func (engine *WorkEngine) runJob(token string, work Work, topic WorkTopic) { |
There was a problem hiding this comment.
+1 to setupJob and runJob vs startJob.
maleck13
force-pushed
the
work-engine-job-improvements
branch
from
7fc1751
to
502efe8
Compare
shawn-hurley
removed
the
do-not-merge/hold
…initions. Allow jobs to be mocked out at broker level
maleck13
force-pushed
the
work-engine-job-improvements
branch
from
502efe8
to
ee996f4
Compare
|
@jmrodri changes made as requested |
I did not create an issue for this change. If one is needed that is no problem. Also as did this out of scope work, if it is felt that it is not needed then I happy to remove the PR. The things here that is likely important is the initialization of the state. Also I know there is a freeze right now so not expecting this to land immediately
Describe what this PR does and why we need it:
While working on the state changes. I was looking into the jobs and work engine. This pr creates a mockable interface laying some of the ground work to allow us to potentially create e2e tests that run against the API ensuring the correct responses without the need to actually spin up APBS but still execute the key paths in the code. It also reduces duplication on in the jobs code.
Changes proposed in this pull request
WorkFactory