-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Cluster scoped workflow template #2451
feat: Cluster scoped workflow template #2451
Conversation
Co-Authored-By: Simon Behar <simbeh7@gmail.com>
I've requested a few small changes - but given approval anyway to move this forward. |
Please hold off on merging this, I'm adding some review comments. |
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
|
||
// GetTemplateScope returns the template scope of workflow template. | ||
func (cwftmpl *ClusterWorkflowTemplate) GetTemplateScope() string { | ||
return "cluster/" + cwftmpl.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor and pedantic comment, but we use different grammar with cluster/<template_name>
and namespaced/<template_name>
.
In my opinion it should be either:
clustered/<template_name>
andnamespaced/<template_name>
; orcluster/<template_name>
andnamespace/<template_name>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just following Kubernetes conventions
# either Namespaced or Cluster
scope: Namespaced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ResourceScope is an enum defining the different scopes available to a custom resource
type ResourceScope string
const (
ClusterScoped ResourceScope = "Cluster"
NamespaceScoped ResourceScope = "Namespaced"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jessesuen I would like to get your feedback on this before merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this GetTemplateScope
method even being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, based on the name of this method, I would expect it to return a ResourceScope
type of either Cluster
or Namespaced
, but I see it's the scope + name of the resource, which they could already get by metadata.name
Can I learn the intended use of this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is used in multiple where the controller tries to load the stored template.
https://github.com/argoproj/argo/blob/184c3653085bc8821bdcd65f5476fbe24f24b00e/workflow/templateresolution/context.go#L152
https://github.com/argoproj/argo/blob/fb74ba1ce27b96473411c2c5cfe9a86972af589e/workflow/controller/steps.go#L199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I thought it was being introduced in this change, but could not find the usage in this PR. In any case, I think following kubernetes scope naming of cluster
and namespaced
is the right choice.
That said, I do believe it would be better to make GetTemplateScope()
return a ResourceScope kind, change GetStoredTemplate()
to accept both a scope and name, and then inside GetStoredTemplate()
, formulate the map key based on concatenating the two with a slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agreed with you. But TemplateScope
word is used across workflowtemplate implantation and workflow spec. Instead of changing one place. I would recommend adding a new function that will return templateBaseID (scope + name) and rename variable, arguments (except spec change). I would like to do it in separate PR, so, it will be easier to review.
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
. Creating WorkflowTemplate on a cluster level so all namespace can use it #1729