resource template merge & strategic patch error about using both -f and -p

[djquiambao18]

Hello,
in

argo-workflows/workflow/executor/resource.go

Line 137 in 06da23e

appendFileFlag = false

the -f flag is not added for mergeStrategy: json but any others add this per the appendFileFlag. In the comments preceding Line 137, it seems that the implementer was aware of this limitation.

However, just a few lines down:

argo-workflows/workflow/executor/resource.go

Line 144 in 06da23e

args = append(args, "-p")

the -p is still added along with the -f flag later on, inevitably causing the "patch" action to be broken.

For instance, when you try to use patch action with resource something like this example: https://github.com/argoproj/argo-workflows/blob/main/examples/k8s-patch.yaml - (my use-case is with ConfigMap)

the engine parses it into a kubectl command like:

level=info msg=kubectl patch --type strategic -p '{\"data\": {\"azure-dev\": \"true\"}}'\n configmap deploy-status -f /tmp/manifes
t.yaml -o json

which will give an error:

level=error msg="executor error: no more retries error: when paths, URLs, or stdin is provided as input, you may not specify resource arguments as well"

Is this the intended behavior and am I simply misunderstanding something here? It seems that the only option to prevent this issue is to use the json mergeStrategy at the moment.

Thanks and clarification is appreciated!

[agilgur5]

It looks like someone else made a PR to attempt to change this, but they realized it was incorrect: #11249 (comment):

The current way to do it is to write instead

      resource:
        action: patch
        mergeStrategy: merge
        manifest: |
          apiVersion: v1
          kind: ConfigMap
          metadata:
            name: name-of-configmap
          data:
            test: "Goodbye"

[agilgur5]

Mm is the problem that merge and strategic accept both a full manifest and a partial one?
As such the spec doesn't know which one you're trying to use?

If so, I guess we might be able to infer a partial based on the manifest (e.g. lack of metadata.name etc) or other flags. PR welcome

[agilgur5]

Hmm, we might just be able to use --patch-file universally for all patches instead of -f

[agilgur5]

Threw up a draft PR of that^: #13317. Still needs some testing though

[agilgur5]

Fixed in #13317.
This discussion also effectively ends up as a duplicate of the now fixed #11248

More details:
--patch-file universally doesn't quite work and has the same problem as -p.

When a full manifest is given, effectively both -f and --patch-file are needed, pointing to the same manifest. -f gets the kind and name from the manifest (the "header") and --patch-file uses the manifest as a patch file.

So I instead used the strategy I outlined above to infer a "full" manifest with a header by inspecting if it has a kind.