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

Weird argument arrangement when using script template #4481

Closed
ivancili opened this issue Nov 6, 2020 · 4 comments · Fixed by #4492
Closed

Weird argument arrangement when using script template #4481

ivancili opened this issue Nov 6, 2020 · 4 comments · Fixed by #4492
Labels
solution/workaround There's a workaround, might not be great, but exists type/feature Feature request

Comments

@ivancili
Copy link
Contributor

ivancili commented Nov 6, 2020

Summary

I wanted to add a template that is activated on a workflow exit (the onExit field) with a purpose of notifying the user about the workflow status. I decided to use a template of type script, like this:

name: "workflow-status-test"
script: 
 image: "python:3.6-alpine"
 command:
  - python3
 args:
  - "{{workflow.status}}"
 source: "import sys; import my_notify_func; my_notify_func(sys.argv[1])"

This failed. After inspecting the pod, I saw that Argo passed the arguments to the python container in the following way Succeeded /argo/staging/script, I would expect it to do it like this /argo/staging/script Succeeded.

Later I noticed in one of your examples in git repo that I could embed a template directly in the source field. But this wasn't documented anywhere (at least I didnt see it). So, this solves my issue:

name: "workflow-status-test"
script: 
 image: "python:3.6-alpine"
 command:
  - python3
 source: "import my_notify_func; my_notify_func('{{workflow.status}}')"

The order in which the arguments are passed is unintuitive and I don't see a case where this particular order would be useful. My suggestion is to change the order (so first goes the script path, and then the user args) or put a notice/disclaimer in the docs. Also, you could provide an example for using templates here, or document it.

Use Cases

When would you use this?


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@ivancili ivancili added the type/feature Feature request label Nov 6, 2020
@alexec
Copy link
Contributor

alexec commented Nov 6, 2020

Why not do this:

name: "workflow-status-test"
script: 
 image: "python:3.6-alpine"
 command:
  - python3
 source: |
   import sys; import my_notify_func; my_notify_func("{{workflow.status}}")

@alexec alexec added the solution/workaround There's a workaround, might not be great, but exists label Nov 6, 2020
@ivancili
Copy link
Contributor Author

ivancili commented Nov 7, 2020

@alexec Yes, as I said, that works just fine and is more than enough for my use case.

All I'm saying is that this behavior is unexpected/unintuitive. As soon as someone tries to use args field in a script template, they will definitely encounter the same problem. Because no one would expect args to be passed directly to the command, instead of passing them to the script. It's not that big of a deal really, I would probably just document it in "Field reference" page and provide an example with templates under "Examples".

@alexec
Copy link
Contributor

alexec commented Nov 7, 2020

Got it. Would you like to submit a PR to fix?

@ivancili
Copy link
Contributor Author

ivancili commented Nov 7, 2020

@alexec Sure

alexec pushed a commit that referenced this issue Nov 10, 2020
…olves #4481 (#4492)

Signed-off-by: ivancili <ivandotilic@gmail.com>
alexcapras pushed a commit to alexcapras/argo that referenced this issue Nov 12, 2020
…olves argoproj#4481 (argoproj#4492)

Signed-off-by: ivancili <ivandotilic@gmail.com>
Signed-off-by: Alex Capras <alexcapras@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution/workaround There's a workaround, might not be great, but exists type/feature Feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants