Use utils.command to replace exec.command #3686
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3686 +/- ##
==========================================
- Coverage 64.47% 64.46% -0.01%
==========================================
Files 471 472 +1
Lines 28140 28180 +40
==========================================
+ Hits 18142 18165 +23
- Misses 7844 7856 +12
- Partials 2154 2159 +5 ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| // allowPathlist of safe commands | ||
| var allowPathlist = map[string]bool{ |
There was a problem hiding this comment.
How about renaming it to allowCommandList?
| } | ||
|
|
||
| // CheckCommandArgs is check string is valid in args | ||
| func checkCommandArgs(arg ...string) (err error) { |
There was a problem hiding this comment.
I think we can simplify this by using strings.ContainsAny()
There was a problem hiding this comment.
I think it can indicate which char is illegalChar.
pkg/utils/helm/helm.go
Outdated
| @@ -93,7 +93,10 @@ func GenerateHelmTemplate(name string, namespace string, valueFileName string, c | |||
| // return syscall.Exec(cmd, args, env) | |||
| // 5. execute the command | |||
| log.V(1).Info("Generating template", "args", args) | |||
| cmd := exec.Command("bash", "-c", strings.Join(args, " ")) | |||
| cmd, err := utils.SafeCommand("bash", "-c", strings.Join(args, " ")) | |||
There was a problem hiding this comment.
This seems in a unused function? How about removing the function?
|
|
||
| // allowPathlist of safe commands | ||
| var allowPathlist = map[string]bool{ | ||
| // "helm": true, |
There was a problem hiding this comment.
Why not adding ddc-helm to the white-list?
There was a problem hiding this comment.
Thanks, I forgot it.
pkg/utils/exec.go
Outdated
| // allowPathlist of safe commands | ||
| var allowPathlist = map[string]bool{ | ||
| // "helm": true, | ||
| "kubectl": true, |
There was a problem hiding this comment.
I think we can use func init() to find the kubectl and ddc-helm command path, and put them into the white list dynamically
Signed-off-by: cheyang <[email protected]> Remove unused method, To #53506158 Signed-off-by: cheyang <[email protected]>
cheyang
force-pushed
the
mv_exec_command_into_helper
branch
from
3dbe585
to
15d8a40
Compare
Signed-off-by: cheyang <[email protected]>
Signed-off-by: cheyang <[email protected]>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TrafalgarZZZ 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 |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews