SDK - Compiler - Add optional Argo validation #3094
Conversation
argo CLI tool must be in path for this feature to work
|
/retest |
|
IIUC after this change it's possible to generate an invalid workflow spec, right? Is there any use case where we need such whitelisting? Thanks |
After this change it should be impossible to generate an invalid workflow spec. It relies on the presence of |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: numerology The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add Argo CLI so that the validation introduced in kubeflow#3094 is triggered.
| import subprocess | ||
| argo_path = shutil.which('argo') | ||
| if argo_path: | ||
| result = subprocess.run([argo_path, 'lint', '/dev/stdin'], input=yaml_text, encoding='utf-8', stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Did it (maybe intentionally or accidentally) drop Python 3.5 support, because subprocess.run() does not seem to have encoding arg in python 3.5 [1]. As a result, it would cause TypeError.
[1] https://docs.python.org/3.5/library/subprocess.html#subprocess.run
There was a problem hiding this comment.
Interesting... In Travis CI [1] we do test against Python 3.5 and it does not complain about this change. Will take a closer look to it.
Did you find anything wrong @ucdmkt ?
[1] https://github.com/kubeflow/pipelines/blob/master/.travis.yml#L54
There was a problem hiding this comment.
Okay in Travis, the unit tests we run _compile() instead of compile() Thus this line was not covered.
There was a problem hiding this comment.
Okay in Travis, the unit tests we run
_compile()instead ofcompile()Thus this line was not covered.
Took back my word. It's because in Travis Argo CLI is missing so it's not triggered.
Add Argo CLI so that the validation introduced in #3094 is triggered.
argo CLI tool must be in path for this feature to work
Add Argo CLI so that the validation introduced in kubeflow#3094 is triggered.
Argo CLI tool must be in path for this feature to work
This change is