[Fix #936] Adding Python implementation #1027
Conversation
Signed-off-by: fjtirado <ftirados@redhat.com>
fjtirado
force-pushed
the
Fix_#936
branch
5 times, most recently
from
7d74e17 to
78b8d96
Compare
fjtirado
force-pushed
the
Fix_#936
branch
2 times, most recently
from
7ff3e23 to
8b57ec9
Compare
| private void addArg(List<String> list, String name, Object value) { | ||
| if (value instanceof Boolean bool) { | ||
| if (bool) { | ||
| list.add("--" + name); |
There was a problem hiding this comment.
Because of the way https://github.com/serverlessworkflow/catalog/blob/main/functions/log/1.0.0/function.yaml is implemented, params has to be prefixed with --
Notice that this impose restriction in other python scripts to be invoked, which has to follow the same structure.
Also this is assuming that, for boolean parameters, all python function will use store_true
This kind of conventions are one of the reasons why I feel we should change the spec to redefine scripts as calling embedded named parameters functions (which are supported both by JS and Python)
Signed-off-by: fjtirado <ftirados@redhat.com>
| <module>container</module> | ||
| <module>test</module> | ||
| <module>script-js</module> | ||
| <module>python</module> |
There was a problem hiding this comment.
Why not to follow script-python naming to follow an standard?
There was a problem hiding this comment.
I think that probably js should have been called javascript
mcruzdev
left a comment
mcruzdev
left a comment
There was a problem hiding this comment.
Looks great to me, I just left a comment about the module name.
| WorkflowContext workflowContext, | ||
| TaskContext taskContext, | ||
| WorkflowModel model) { | ||
| ProcessBuilder builder = new ProcessBuilder("python", "-c", scriptContext.code()); |
There was a problem hiding this comment.
It is just a question, why we are using a process instead a embedded python like we did in Javascript? I mean, what is the motivation?
There was a problem hiding this comment.
It is implied as result of #1027 (comment)
The catalog function is forcing this to be invoked as external script.
I do not really like that and I think we need to dicuss on the CNCF forum, but right now we must implement this that way
Also, embedding python will require some intallation procedure in the target machine (the native library to be embedded needs to be compiled by python wheel first), while embedding Javascript is easier because the there are 100% java libraries doing that
3 participants
Fix #936