Gitops fix to include GitOpsTemplate when listing templates #2070
Conversation
ranatrk
added
bug
ranatrk
force-pushed
the
2016-fix-listing-gitops-templates
branch
from
840db45 to
21cdf64
Compare
ranatrk
force-pushed
the
2016-fix-listing-gitops-templates
branch
from
21cdf64 to
9e16bf9
Compare
| if flags.ListTemplateParameters { | ||
| if len(args) == 0 { | ||
| return errors.New("template name is required") | ||
| } | ||
|
|
||
| return templates.GetTemplateParameters(templates.CAPITemplateKind, args[0], flags.TemplateNamespace, client, w) | ||
| //Default template kind for GetTemplateParameters is CAPITemplate | ||
| if templateKind == "" { |
There was a problem hiding this comment.
Under what circumstances is templateKind == "" ?
https://github.com/weaveworks/weave-gitops-enterprise/blob/9e16bf9e47fffad096000e078a269dc3ba92aa65/cmd/gitops/app/get/templates/cmd.go#L93 should prevent this?
Shouldn't it set it to the CAPITemplate kind?
There was a problem hiding this comment.
the DefaultTemplateKind is an empty string, more of a "all" indicator
If its set to CAPITemplate, then when using get templates only CAPITemplates will be retrieved, where if it was empty all types are retrieved (handeled here)
There are 3 cases I tried to handle for geting templates without parameters, [GitOpsTemplate,CAPITemplate, all-supported-templates]
When getting parameters, If trying to get it for all templates then it would fail for some template kinds as the template with the name provided is of only one type, so I defaulted to CAPITemplates
(This was mainly to default to all for get templates, and default to CAPITemplates when getting get templates TEMP_NAME --list-parameters
ranatrk
force-pushed
the
2016-fix-listing-gitops-templates
branch
from
10019c1 to
fca2a05
Compare
There was a problem hiding this comment.
Looks good to me.
Not necessarily for this branch, but perhaps we could look at using https://pkg.go.dev/k8s.io/cli-runtime/pkg/printers ?
(specifically the ResourcePrinter implementations)
And also not for this branch, but what if someone wants a YAML or JSON format version?
Maybe we should open an issue so that we can support
-o, --output='':
Output format. One of: (json, yaml, name, go-template, go-template-file, template, templatefile, jsonpath,
jsonpath-as-json, jsonpath-file, custom-columns, custom-columns-file, wide). See custom columns
[https://kubernetes.io/docs/reference/kubectl/#custom-columns], golang template
[http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template
[https://kubernetes.io/docs/reference/kubectl/jsonpath/].
| *t = TemplateKind(v) | ||
| return nil | ||
|
|
||
| } else { |
There was a problem hiding this comment.
You can simpify this code a bit and remove the else if bits because each clause is returning?
| } | ||
|
|
||
| // Return a string representation of all supported template Kinds | ||
| func TemplateKindsString() string { |
There was a problem hiding this comment.
This doesn't need to be exported, it's not used outside the package?
| return nil | ||
|
|
||
| } else { | ||
| return errors.New(`Template kind not found. Supported templates: ` + TemplateKindsString()) |
There was a problem hiding this comment.
This works, but you could also do the more common:
fmt.Errorf("template kind not found, supported templates: %s", templateKindsString())Generally Go errors don't start with initial caps, as they tend to get concatenated together see https://github.com/golang/go/wiki/CodeReviewComments#error-strings for a bit more on this.
Fix listing templates to include gitopstemplates Add defaults to template kind when listing templates
ranatrk
force-pushed
the
2016-fix-listing-gitops-templates
branch
from
d1d0bf7 to
3a3176f
Compare
Closes #2016
What changed?
template-kindflag added toget templatescmd to filter templates based on the template kindWhy was this change made?
The template kind was previously hard coded to only use CAPITemplates, and so when listing templates, any GitOpsTemplate was not retrieved in the results
How was this change implemented?
Add template-kind flag to get templates with cli
Fix listing templates to include gitopstemplates
Add defaults to template kind when listing templates
How did you validate the change?
Release notes
Documentation Changes