Profile patch with invalid op reports "invalid path" instead of "invalid op"

[TBBle]

Expected behavior

An invalid op in the profile.patches list should complain that the op is invalid, i.e.,

> skaffold dev -p NodePort
applying profiles: applying profile "NodePort": invalid op: delete

Actual behavior

An invalid op in the profile.patches list complains that the path is invalid, i.e.

> skaffold dev -p NodePort
applying profiles: applying profile "NodePort": invalid path: /deploy/helm/releases/0/setValues/service.port

It wasn't until I went to look at the spec after about a half-hour of trying to rewrite, rephrase, or simply respell the path, and then hunting through Skaffold bug reports, that I went to look at the JSON Patch site, and discovered that the op is remove, not delete.

The path is not invalid.

Information

• Skaffold version: v1.17.2
• Operating system: Microsoft Windows [Version 10.0.19041.685]
• Contents of skaffold.yaml:

apiVersion: skaffold/v2beta10
kind: Config
metadata:
  name: my-server
build:
  artifacts:
    - image: my-server
      docker: {}
deploy:
  helm:
    releases:
      - name: my-server
        chartPath: charts/my-server
        artifactOverrides:
          image: my-server
        setValues:
          service.type: LoadBalancer
          service.port: 8000
profiles:
  - name: NodePort
    patches:
      - op: replace
        path: /deploy/helm/releases/0/setValues/service.type
        value: NodePort
      - op: delete
        path: /deploy/helm/releases/0/setValues/service.port

Steps to reproduce the behavior

1. an empty directory containing only the skaffold.yaml above.
2. skaffold dev -p NodePort
3. Profit

[TBBle]

The fix should be simple enough. Instead of making up an error if anything is wrong with the patch attempt, perhaps skaffold should propagate the error we got from yamlpatch and show the user that.

If possible, having the config schema actually validate that the optional op is one of the allowed values would also have caught this, I believe. I don't see that capacity right now, but perhaps adding something like yamltags:"enum=add=remove=replace=move=copy=test" might be worthwhile?

[tejal29]

Thanks @TBBle for opening an issue with fixes.
Please let us know on how we could help you get the change in faster.