-
Notifications
You must be signed in to change notification settings - Fork 122
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
remove ContainerOp from test cases - part 3 #693
Conversation
486c18b
to
4a8c3aa
Compare
taskSpec: | ||
steps: | ||
- name: main | ||
args: | ||
- sleep $(inputs.params.sleep_ms); wget localhost:5678 -O $(results.downloaded-resultoutput.path) | ||
- | | ||
sleep None; wget localhost:5678 -O $0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be sleep $(inputs.params.sleep_ms);
. Do you think valuable replacement for image works in component.yaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it doesn't work. then we need to fix the compiler error. let me revert this and put a comment on issue 670
- -c | ||
args: | ||
- | | ||
import random; import sys; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we need a new line here? when compiling it, the newline makes the spacing look very bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, merge it with next line
@@ -74,7 +85,7 @@ spec: | |||
steps: | |||
- name: main | |||
args: | |||
- echo "$0" | |||
- echo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick question, does this command still work without "$0"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, for the old one, it uses parameter substitution. the new one just two arguments. both produce the same output
eliminate the usage of ContainerOp in testcases and update the testcase accordingly, including filenames starting with `o` to `u` Signed-off-by: Yihong Wang <yh.wang@ibm.com>
4a8c3aa
to
84b6c0e
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Tomcli, yhwang 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 |
eliminate the usage of ContainerOp in testcases and
update the testcase accordingly, including filenames starting
with
o
tou
related to #670
Signed-off-by: Yihong Wang yh.wang@ibm.com