feat: added namespace as cache scope #6342
Conversation
capri-xiyue
changed the title
| if task.GetNamespace() == "" { | ||
| return errMustSpecify("Namespace") | ||
| } |
There was a problem hiding this comment.
this PR will require a coordinated release with SDK, because 1.7.0 SDK launchers will no longer work.
I think we can go with this too, but it might be slightly better to keep namespace as optional, because usually people will not expect 1.7.1 SDK works with the backend, but 1.7.0 SDK doesn't. We can change it to be required after SDK 1.8.0
There was a problem hiding this comment.
Why it won't work with sdk? Launcher is the only caller of such apis.
| if strings.HasPrefix(task.GetPipelineName(), "namespace/") { | ||
| s := strings.Split(task.GetPipelineName(), "/") | ||
| if len(s) != 4 { | ||
| return util.NewInvalidInputError("invalid PipelineName for namespaced pipelines, need to follow 'namespace/${namespace}/pipeline/${pipelineName}': %s", task.GetPipelineName()) | ||
| } |
There was a problem hiding this comment.
note, we don't have any restrictions on pipeline name so far, so it may contain "/".
Recommend using https://pkg.go.dev/strings#SplitN instead.
v2/component/launcher.go
Outdated
| if strings.HasPrefix(o.PipelineName, "namespace/") { | ||
| s := strings.Split(o.PipelineName, "/") | ||
| if len(s) != 4 { | ||
| return fmt.Errorf("invalid PipelineName options for namespaced pipelines, need to follow 'namespace/${namespace}/pipeline/${pipelineName}': %s", o.PipelineName) | ||
| } | ||
| namespace := s[1] | ||
| if namespace != o.Namespace { | ||
| return fmt.Errorf("the namespace %s extracted from pipelineName is not equal to the namespace %s in launcher options", namespace, o.Namespace) | ||
| } | ||
| } |
There was a problem hiding this comment.
For discussion: is it helpful adding the validation in launcher? I feel like launcher can just be unaware of the pipeline name format, if there's a problem. It'll be reported at task server.
There was a problem hiding this comment.
Info like "PipelineName" and "Namespace" will get recorded in MLMD when cache is not enabled. I think launcher should also be aware of the validation.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy 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:
Approvers can indicate their approval by writing |
|
/retest |
part of #6324
Description of your changes:
added namespace as cache scope
Checklist: