Skip to content
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

fix(cli): handle multi-resource yaml in offline lint. Fixes #12137 #13531

61 changes: 61 additions & 0 deletions cmd/argo/commands/lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,4 +221,65 @@ spec:
require.NoError(t, err)
assert.False(t, fatal, "should not have exited")
})

workflowMultiDocsPath := filepath.Join(subdir, "workflowMultiDocs.yaml")
err = os.WriteFile(workflowMultiDocsPath, []byte(`
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
name: hello-world-template-local-arg-1
spec:
templates:
- name: hello-world
inputs:
parameters:
- name: msg
value: 'hello world'
container:
image: busybox
command: [echo]
args: ['{{inputs.parameters.msg}}']
---
apiVersion: argoproj.io/v1alpha1
kind: WorkflowTemplate
metadata:
name: hello-world-template-local-arg-2
spec:
templates:
- name: hello-world
inputs:
parameters:
- name: msg
value: 'hello world'
container:
image: busybox
command: [echo]
args: ['{{inputs.parameters.msg}}']
---
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: hello-world-local-arg-
spec:
entrypoint: whalesay
templates:
- name: whalesay
steps:
- - name: hello-world
templateRef:
name: hello-world-template-local-arg-2
template: hello-world
`), 0644)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be rewritten for #13610, but that should probably be a separate PR for the whole file. Your current style matches the existing style of this file, so I think this is fine for now

require.NoError(t, err)

t.Run("linting a workflow in multi-documents yaml", func(t *testing.T) {
defer func() { logrus.StandardLogger().ExitFunc = nil }()
var fatal bool
logrus.StandardLogger().ExitFunc = func(int) { fatal = true }
err = runLint(context.Background(), []string{workflowMultiDocsPath}, true, nil, "pretty", false)

require.NoError(t, err)
assert.False(t, fatal, "should not have exited")
})

}
60 changes: 30 additions & 30 deletions pkg/apiclient/offline-client.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ import (
"github.com/argoproj/argo-workflows/v3/pkg/apiclient/workflowtemplate"
wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1"
"github.com/argoproj/argo-workflows/v3/util/file"
"github.com/argoproj/argo-workflows/v3/workflow/common"
"github.com/argoproj/argo-workflows/v3/workflow/templateresolution"

"sigs.k8s.io/yaml"
)

type offlineWorkflowTemplateGetterMap map[string]templateresolution.WorkflowTemplateNamespacedGetter
Expand Down Expand Up @@ -50,42 +49,43 @@ func newOfflineClient(paths []string) (context.Context, Client, error) {

for _, basePath := range paths {
err := file.WalkManifests(basePath, func(path string, bytes []byte) error {
var generic map[string]interface{}
if err := yaml.Unmarshal(bytes, &generic); err != nil {
return fmt.Errorf("failed to parse YAML from file %s: %w", path, err)
}
switch generic["kind"] {
case "ClusterWorkflowTemplate":
cwftmpl := new(wfv1.ClusterWorkflowTemplate)
if err := yaml.Unmarshal(bytes, &cwftmpl); err != nil {
return fmt.Errorf("failed to unmarshal file %s as a ClusterWorkflowTemplate: %w", path, err)
for _, pr := range common.ParseObjects(bytes, false) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is copied from lint.go, so similarly I'd say this is fine for now, but a more clear name would be better in both places, like parsed instead of pr (which makes one think of "pull request")

Suggested change
for _, pr := range common.ParseObjects(bytes, false) {
for _, parsed := range common.ParseObjects(bytes, false) {

obj, err := pr.Object, pr.Err
if err != nil {
return fmt.Errorf("failed to parse YAML from file %s: %w", path, err)
}

if _, ok := clusterWorkflowTemplateGetter.clusterWorkflowTemplates[cwftmpl.Name]; ok {
return fmt.Errorf("duplicate ClusterWorkflowTemplate found: %q", cwftmpl.Name)
if obj == nil {
continue // could not parse to kubernetes object
Comment on lines +58 to +59

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too is copied from lint.go, although I would think both places should have an error in this case rather than a silent continue.

}
clusterWorkflowTemplateGetter.clusterWorkflowTemplates[cwftmpl.Name] = cwftmpl

case "WorkflowTemplate":
wftmpl := new(wfv1.WorkflowTemplate)
if err := yaml.Unmarshal(bytes, &wftmpl); err != nil {
return fmt.Errorf("failed to unmarshal file %s as a WorkflowTemplate: %w", path, err)
}
getter, ok := workflowTemplateGetters[wftmpl.Namespace]
if !ok {
getter = &offlineWorkflowTemplateNamespacedGetter{
namespace: wftmpl.Namespace,
workflowTemplates: map[string]*wfv1.WorkflowTemplate{},
objName := obj.GetName()
namespace := obj.GetNamespace()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here that this is copied from lint.go, but the naming could be consistent with objName as objNs in both places

Suggested change
namespace := obj.GetNamespace()
objNs := obj.GetNamespace()


switch v := obj.(type) {
case *wfv1.ClusterWorkflowTemplate:
if _, ok := clusterWorkflowTemplateGetter.clusterWorkflowTemplates[objName]; ok {
return fmt.Errorf("duplicate ClusterWorkflowTemplate found: %q", objName)
}
clusterWorkflowTemplateGetter.clusterWorkflowTemplates[objName] = v

case *wfv1.WorkflowTemplate:
getter, ok := workflowTemplateGetters[namespace]
if !ok {
getter = &offlineWorkflowTemplateNamespacedGetter{
namespace: namespace,
workflowTemplates: map[string]*wfv1.WorkflowTemplate{},
}
workflowTemplateGetters[namespace] = getter
}
workflowTemplateGetters[wftmpl.Namespace] = getter
}

if _, ok := getter.(*offlineWorkflowTemplateNamespacedGetter).workflowTemplates[wftmpl.Name]; ok {
return fmt.Errorf("duplicate WorkflowTemplate found: %q", wftmpl.Name)
if _, ok := getter.(*offlineWorkflowTemplateNamespacedGetter).workflowTemplates[objName]; ok {
return fmt.Errorf("duplicate WorkflowTemplate found: %q", objName)
}
getter.(*offlineWorkflowTemplateNamespacedGetter).workflowTemplates[objName] = v
}
getter.(*offlineWorkflowTemplateNamespacedGetter).workflowTemplates[wftmpl.Name] = wftmpl
}

}
return nil
})

Expand Down
Loading