Add --dry-run option to the submit command

[AnesBenmerzoug]

This PR is an attempt to resolve issue #1217
It adds a --dry-run option to the submit command.

As an example, running this command:

argo submit https://raw.githubusercontent.com/argoproj/argo/master/examples/hello-world.yaml --dry-run -o yaml

Produces the following output:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  creationTimestamp: "2019-07-26T22:54:34Z"
  generateName: hello-world-
  generation: 1
  name: hello-world-5vz5l
  namespace: default
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/default/workflows/hello-world-5vz5l
  uid: 5643ab48-aff8-11e9-b70b-080027bc9e0f
spec:
  arguments: {}
  entrypoint: whalesay
  templates:
  - container:
      args:
      - hello world
      command:
      - cowsay
      image: docker/whalesay:latest
      name: ""
      resources: {}
    inputs: {}
    metadata: {}
    name: whalesay
    outputs: {}
status:
  finishedAt: null
  startedAt: null

Feedback would be much appreciated.
Some of the changes were inspired by the work done in PR #1312

[CLAassistant]

All committers have signed the CLA.

[xianlubird]

Can you provide a example for argo submit xxx.file --entrypoint, can dry-run show the override entrypoint. And what is the scene of your use of this feature?

[AnesBenmerzoug]

@xianlubird Running the following command on the command loops-maps.yaml example manifest:

argo submit https://raw.githubusercontent.com/argoproj/argo/master/examples/loops-maps.yaml  --dry-run -o yaml --entrypoint cat-os-release -p image=debian -p tag=9.1

Gives:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  creationTimestamp: "2019-07-29T18:33:31Z"
  generateName: loops-maps-
  generation: 1
  name: loops-maps-bzfmt
  namespace: default
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/default/workflows/loops-maps-bzfmt
  uid: 5dbe25aa-b22f-11e9-bf04-080027bc9e0f
spec:
  arguments:
    parameters:
    - name: image
      value: debian
    - name: tag
      value: "9.1"
  entrypoint: cat-os-release
  templates:
  - inputs: {}
    metadata: {}
    name: loop-map-example
    outputs: {}
    steps:
    - - arguments:
          parameters:
          - name: image
            value: '{{item.image}}'
          - name: tag
            value: '{{item.tag}}'
        name: test-linux
        template: cat-os-release
        withItems:
        - image: debian
          tag: "9.1"
        - image: debian
          tag: "8.9"
        - image: alpine
          tag: "3.6"
        - image: ubuntu
          tag: "17.10"
  - container:
      args:
      - /etc/os-release
      command:
      - cat
      image: '{{inputs.parameters.image}}:{{inputs.parameters.tag}}'
      name: ""
      resources: {}
    inputs:
      parameters:
      - name: image
      - name: tag
    metadata: {}
    name: cat-os-release
    outputs: {}
status:
  finishedAt: null
  startedAt: null

The dry-run option can be used to see the final manifest before applying it.
Or its result can be stored for debugging in case something breaks and someone would like to see what was applied.

Edit:
Corrected the output of the dry run

[xianlubird]

See my comments

[xianlubird]

Why now use wfClient

[AnesBenmerzoug]

I couldn't access the rest client from wfClient in order to make a POST request.

[xianlubird]

Why not keep the origin code ?

[AnesBenmerzoug]

It seemed confusing. I can revert the names if that is a problem

[xianlubird]

We can get namespace from wf object

[xianlubird]

Why not wfClientset

[xianlubird]

same

[xianlubird]

Is your format is correct ?

[AnesBenmerzoug]

I applied both gofmt and goimports and checked with the linter.

[xianlubird]

Will this action do the real POST thing , the workflow will be created in the code

[AnesBenmerzoug]

I followed the API documentation and added the dryRun=All parameter which will run checks to make sure the request is valid, mutates the request, merges patches, defaults field values, and validates the schema validation. It shouldn't create anything.

All of that will be done on the server side and is only possible, by default, on kubernetes versions >= 1.13 ( has to be enabled for version 1.12 )

[sarabala1979]

Here code duplication and hard to maintain. you can change the create function and add dry-run optional parameter.

[AnesBenmerzoug]

The Create method was generated using client-gen.

I didn't want to change the method since it specifically says at the top of the file:

// Code generated by client-gen. DO NOT EDIT.

[sarabala1979]

got it

[sarabala1979]

Output of argo submit examples/hello-world.yaml --dry-run is misleading. if option --dry-run, output should be an empty or simple message "output option is missing"

[AnesBenmerzoug]

@sarabala1979 You're right, it does look confusing.
For the printing, I just reused the PrintWorkflow function defined here.
It was already used in the submit command.

It displays by default the wide output because in the code the same case is used when for wide and no output format i.e. an empty string.

We could change the default value to just print the name instead of the wide output. But I don't feel too confident changing a default that users may be used to.

[sarabala1979]

you can add validation for dry-run without -o/---output option.
if submitOpts.DryRun && cliOpts.output == "" { fmt.Println("Dry-run should have output option") return }

[xianlubird]

For k8s version < 1.12, without feature gate enable. will this post the real action to the apiServer ?

Should we limit this action on k8s version < 1.12

[AnesBenmerzoug]

I tried on a few different versions of kubernetes on minikube and this is what I got:

• On v1.10.13: It does not complain and creates the resources ignoring completely the --dry-run option
• On v1.12.10: Failed to submit workflow: the dryRun alpha feature is disabled
• On >= v1.13: It works as expected

It seems that a check should be added when using --dry-run with an kubernetes version older than 1.12

[AnesBenmerzoug]

you can add validation for dry-run without -o/---output option.
if submitOpts.DryRun && cliOpts.output == "" { fmt.Println("Dry-run should have output option") return }

@sarabala1979 You're right. I went ahead and added your suggestion

[sarabala1979]

LGTM @jessesuen can you take a quick look?

[jessesuen]

There are two types of dry-runs in kubernetes, server-side and client. This PR appears to be implementing server-side. In which case, the flag should be named --server-dry-run.

I actually think there's not really much of the value for having the server side dry-run, since the output of that I believe is exactly the same as client side.

$ kubectl apply --help | grep dry
      --dry-run=false: If true, only print the object that would be sent, without sending it. Warning: --dry-run cannot accurately output the result of merging the local manifest and the server-side data. Use --server-dry-run to get the merged result instead.
      --server-dry-run=false: If true, request will be sent to server with dry-run flag, which means the modifications won't be persisted. This is an alpha feature and flag.

I think for this PR, we should be implementing client-side dry run as the required feature, and server-side as an optional. Client-side is more useful since API server access is not necessary.

[AnesBenmerzoug]

@jessesuen Thanks for the feedback.
You're right. This is currently implements only the server-side dry-run and I think that it's mostly ready except for two things:

• Renaming it to server-dry-run
• Adding a check for kubernetes versions older than v1.12

As for the client-side dry-run, it seems that, from what I saw in the kubectl code, to be a bit more involved and complex than the server-side dry-run and thus would take longer to implement.

[jessesuen]

As for the client-side dry-run, it seems that, from what I saw in the kubectl code, to be a bit more involved and complex than the server-side dry-run and thus would take longer to implement.

We don't need to follow what kubetl client side dry-run is doing. The primary ask, for this issue is for someone to benefit from all of the substitution that argo CLI is doing such as changing entrypoint, overriding generate name, giving instance id, etc... These substitutions are what we do in SubmitWorkflow, the user just needs to get the YAML from it without actually going through with it.

The only change you would need to make, is add a new condition to check for client side --dry-run here:
https://github.com/argoproj/argo/pull/1506/files#diff-ee4f8f7ad262589b16b9a2844f2f7aa5R249

And return the workflow object without submitting it at all.

[AnesBenmerzoug]

@jessesuen You're right. That is much simpler than what I had in mind.

Doing that and running it on the loops-maps example I got:

• For --dry-run:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  creationTimestamp: null
  generateName: loops-maps-
  namespace: default
spec:
  arguments: {}
  entrypoint: loop-map-example
  templates:
  - inputs: {}
    metadata: {}
    name: loop-map-example
    outputs: {}
    steps:
    - - arguments:
          parameters:
          - name: image
            value: '{{item.image}}'
          - name: tag
            value: '{{item.tag}}'
        name: test-linux
        template: cat-os-release
        withItems:
        - image: debian
          tag: "9.1"
        - image: debian
          tag: "8.9"
        - image: alpine
          tag: "3.6"
        - image: ubuntu
          tag: "17.10"
  - container:
      args:
      - /etc/os-release
      command:
      - cat
      image: '{{inputs.parameters.image}}:{{inputs.parameters.tag}}'
      name: ""
      resources: {}
    inputs:
      parameters:
      - name: image
      - name: tag
    metadata: {}
    name: cat-os-release
    outputs: {}
status:
  finishedAt: null
  startedAt: null

• For --server-dry-run:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  creationTimestamp: "2019-07-31T21:56:27Z"
  generateName: loops-maps-
  generation: 1
  name: loops-maps-vtsgs
  namespace: default
  selfLink: /apis/argoproj.io/v1alpha1/namespaces/default/workflows/loops-maps-vtsgs
  uid: 073061c3-bad9-4b2e-9472-1edcb352aa06
spec:
  arguments: {}
  entrypoint: loop-map-example
  templates:
  - inputs: {}
    metadata: {}
    name: loop-map-example
    outputs: {}
    steps:
    - - arguments:
          parameters:
          - name: image
            value: '{{item.image}}'
          - name: tag
            value: '{{item.tag}}'
        name: test-linux
        template: cat-os-release
        withItems:
        - image: debian
          tag: "9.1"
        - image: debian
          tag: "8.9"
        - image: alpine
          tag: "3.6"
        - image: ubuntu
          tag: "17.10"
  - container:
      args:
      - /etc/os-release
      command:
      - cat
      image: '{{inputs.parameters.image}}:{{inputs.parameters.tag}}'
      name: ""
      resources: {}
    inputs:
      parameters:
      - name: image
      - name: tag
    metadata: {}
    name: cat-os-release
    outputs: {}
status:
  finishedAt: null
  startedAt: null

They are mostly the same. The only thing missing is the name when using --dry-run.
I will add some code to generate one from generateName to make it also work with kubectl apply

[jessesuen]

They are mostly the same. The only thing missing is the name when using --dry-run.
I will add some code to generate one from generateName to make it also work with kubectl apply

Actually this is not necessary, and inconsistent with kubectl create --dry-run. We should not populate a name if they only define generateName since this is the responsibility of the server

[AnesBenmerzoug]

@jessesuen I added the client dry-run which just prints the workflow right before the call to Create.
Anything else to add, change, or remove?

[xianlubird]

Can you add a comment for wfClientset , to show that this is only for dry-run now ?

[AnesBenmerzoug]

Done

[jessesuen]

LGTM. Just resolve conflict and it should be good to go

[AnesBenmerzoug]

Thanks @jessesuen, @xianlubird and @sarabala1979 for the review.
I will be travelling to Alaska for a week. I will resolve the conflicts after I come back

[jessesuen]

Conflict was easy to resolve via github editor. Thanks for your contribution!

[AnesBenmerzoug]

@jessesuen thanks for taking care of it 😄