argo lint outputs YAML to JSON conversion error twice when it's used with --offline option

[pakio]

Pre-requisites

• I have double-checked my configuration
• I have tested with the :latest image tag (i.e. quay.io/argoproj/workflow-controller:latest) and can confirm the issue still exists on :latest. If not, I have explained why, in detail, in my description below.
• I have searched existing issues and could not find a match for this bug
• I'd like to contribute the fix myself (see contributing guide)

What happened? What did you expect to happen?

argo lint prints YAML to JSON conversion error twice when it's used with --offline option.

Version(s)

741ab0e

Paste a minimal workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflow that uses private images.

Here's the command to reproduce the error


❯ argo lint hello-world-invalid.yaml
ERRO[2025-01-04T17:11:01.928Z] yaml file at index 0 is not valid: error converting YAML to JSON: yaml: line 16: found a tab character that violates indentation
✖ found nothing to lint in the specified paths, failing...
❯ argo lint --offline hello-world-invalid.yaml
ERRO[2025-01-04T17:11:17.510Z] yaml file at index 0 is not valid: error converting YAML to JSON: yaml: line 16: found a tab character that violates indentation
ERRO[2025-01-04T17:11:17.510Z] yaml file at index 0 is not valid: error converting YAML to JSON: yaml: line 16: found a tab character that violates indentation
✖ found nothing to lint in the specified paths, failing...


yaml file used to verify the result
```yaml
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: hello-world-
  labels:
    workflows.argoproj.io/archive-strategy: "false"
  annotations:
    workflows.argoproj.io/description: |
      This is a simple hello world example.
spec:
  entrypoint: hello-world
  templates:
  - name: hello-world
    container:
      image: busybox
        command: [echo]     # invalid indent
        args: ["hello world"] # invalid indent

### Logs from the workflow controller

```text
N/A (it only happens when argo lint is called with offline mode)

Logs from in your workflow's wait container

N/A

[pakio]

📝 Quick investigation note
When argo lint is used with --offline argument, it executes common.ParseObjects twice, once when newOfflineClient is called (code) and once on lintData (code), where as it's called once on lintData for non-offline clients.

In ParseObjects it try to parse YAML and convert to JSON, then produces error log when it detected some error.

argo-workflows/workflow/common/parse.go

Line 50 in ef41f83

log.Errorf("yaml file at index %d is not valid: %s", i, err)

[chansuke]

I'd like to work on this issue

[pakio]

@chansuke Thank you for raising PR for my issue, but removing support of the offline client doesn't feel a right option for me.
One solution I was considering was to revisit when to verify the YAML file's validity. Since offline client is only been used by argo lint at this moment we might be able to skip the validity check on newOfflineClient, but wanted to make sure that I understand the background of they were added in the first place.

[pakio]

So the common.ParseObjects in newOfflineClient is introduced in this PR #13531, to parse multi-resource yaml for creating a workflow template getter.
But I noticed that along with that change it also changed the behavior of how they treat invalid YAML files. Here's how it behaved before the change, cli built with 7a33720. It immediately fails after detecting invalid file.

./dist/argo lint --offline invalid.yaml
Error: failed to parse YAML from file /home/pakio/program/oss/tmp/hello-world-invalid.yaml: error converting YAML to JSON: yaml: line 16: found a tab character that violates indentation
Usage:
  argo lint FILE... [flags]
...

I prefer the current behavior (warn and continue to the next file rather than failing the whole process) as invalid format is caught in lintData in anyway, but would like to hear community's opinion to make it the best for everyone.

[chansuke]

@pakio Thank you so much for your feedback and sorry for the misunderstanding. And yes, I understood the structure now and agree your suggestion:

would like to hear community's opinion to make it the best for everyone

[pakio]

@chansuke Are you still willing to fix this issue? If you'd like to focus on any other task I can take this over.

Since there were no feedback from the others for about a month, I suppose it would be acceptable to proceed with whatever works properly.
For me personally, as I stated before, I'd like to keep the current behavior. Though it's not a strong preference as long as it can capture the error properly.