feature: support runtime args #2085
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2085 +/- ##
==========================================
- Coverage 65% 64.93% -0.08%
==========================================
Files 209 210 +1
Lines 16227 16257 +30
==========================================
+ Hits 10548 10556 +8
- Misses 4370 4384 +14
- Partials 1309 1317 +8
|
pkg/runtime/path.go
Outdated
| dataSlice = append(dataSlice, arg) | ||
| } | ||
|
|
||
| if err := ioutil.WriteFile(script, []byte(strings.Join(dataSlice, " ")), runtimeScriptPerm); err != nil { |
There was a problem hiding this comment.
All of the parameters should be passed to the runtime. Could you please add $@ to the script？
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/alibaba/pouch/apis/types" |
There was a problem hiding this comment.
I do not think a package in pkg should import package github.com/alibaba/pouch/apis/types. If we must do, I am afraid moving this package out of pkg is better.
| data := fmt.Sprintf("#!/bin/sh\n%s %s $@\n", r.Path, strings.Join(r.RuntimeArgs, " ")) | ||
|
|
||
| if err := ioutil.WriteFile(script, []byte(data), runtimeScriptPerm); err != nil { | ||
| return fmt.Errorf("failed to create runtime script %s: %s", script, err) |
There was a problem hiding this comment.
In this place you directly return the error, while some data in pouchd has been changed, for example, runtimePaths.
In this case, do we need to revert the change of runtimePaths ? just double check.
There was a problem hiding this comment.
if fails here, daemon won't start.
There was a problem hiding this comment.
if fails here, daemon won't start.
Oh, that quite makes sense. Thanks.
pkg/runtime/path.go
Outdated
| return path | ||
| } | ||
|
|
||
| return runtime |
There was a problem hiding this comment.
If it does not exist, return the input runtime?
Will it influence the following code of the calling function GetPath ?
There was a problem hiding this comment.
runtimePaths only keep runtime with path or args
|
Can we add some integration test for this? @Ace-Tang |
|
@allencloud , CI test not suit for this case, since it depend on more runtimes. |
support runtime args pass to pouchd, like
{
"runtimes": {
"cc-runtime": {
"path": "/usr/local/bin/cc-runtime",
"runtimeArgs": [
"--log=/var/log/cc-runtime.log",
"--cc-config=/etc/clear-containers/configuration.toml"
]
}
}
Signed-off-by: Ace-Tang <[email protected]>
|
@allencloud , updated, since runtime not suit for a single package, I move the code into |
|
LGTM |
pouchrobot
added
the
LGTM
5 participants
support runtime args pass to pouchd, like：
Signed-off-by: Ace-Tang [email protected]
Ⅰ. Describe what this PR did
support runtime args set in config.
CI test not suit for this case, since it depend on more runtimes. And the function can be verified in unit test.
Ⅱ. Does this pull request fix one issue?
fixes #2081
fixes #2094
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews