KS-198: Workflow Spec Approval #13181
Conversation
krehermann
force-pushed
the
ks-198/wf-spec-approval
branch
from
e51572c to
137ed53
Compare
krehermann
force-pushed
the
ks-198/wf-spec-approval
branch
from
0a8bc3f to
fabdd61
Compare
krehermann
force-pushed
the
ks-198/wf-spec-approval
branch
from
fabdd61 to
6016360
Compare
krehermann
requested review from
jkongie,
bolekk,
DeividasK,
cedric-cordenier and
HenryNguyen5
core/services/feeds/service.go
Outdated
| @@ -1128,6 +1180,24 @@ func (s *service) generateJob(ctx context.Context, spec string) (*job.Job, error | |||
| js, err = ocrbootstrap.ValidatedBootstrapSpecToml(spec) | |||
| case job.FluxMonitor: | |||
| js, err = fluxmonitorv2.ValidatedFluxMonitorSpec(s.jobCfg, spec) | |||
| case job.Workflow: | |||
| // TODO what the right way to validate a workflow spec? | |||
There was a problem hiding this comment.
This needs to be merged, handling it after CRIB #13125
There was a problem hiding this comment.
what is the eta for that? this feature is largely independent of validation details and needs to be merged for the CLO timeline of EOM. said differently, we can update the validation logic after this if we need to change it
krehermann
force-pushed
the
ks-198/wf-spec-approval
branch
from
e7713a9 to
9659bca
Compare
krehermann
force-pushed
the
ks-198/wf-spec-approval
branch
from
9659bca to
03d1da3
Compare
| func isWFSpec(lggr logger.Logger, spec string) bool { | ||
| jobType, err := job.ValidateSpec(spec) | ||
| if err != nil { | ||
| // this should not happen in practice |
There was a problem hiding this comment.
Seems like there's a lot of duplication around validating specs. I guess it's necessary for each function individually but it appears like it's getting called up to 4 times. Not a big deal but maybe we can refactor and consolidate a bit later
| @@ -1058,6 +1106,11 @@ func (s *service) observeJobProposalCounts(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO KS-205 implement this. Need to figure out how exactly how we want to handle this. | |||
| func findExistingWorkflowJob(ctx context.Context, wfSpec job.WorkflowSpec, tx job.ORM) (int32, error) { | |||
| return 0, nil | |||
There was a problem hiding this comment.
Just want to highlight that without this being implemented, what this means is, if the job has always been managed through CLO then it won't be applicable, as the externalJobID should be able to find the existing job. However if the workflow was added outside CLO then that job needs to be manually removed first. If workflow specs are not being manually added by nops then technically the latter should also not be a problem. However, if we do hide the workflow job from the UI it would also mean that they can't remove the job either.
I don't think it's a problem for now but perhaps we should revisit so it's full proof. The query here is usually through a unique identifier that ties the job to the spec.
|
Add minimal code path for wf spec auto approval via feeds manager