-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix: Remove an unnecessary dependency to sh #145
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ func (f *PluginFlag) BuildPlugins(verbose bool) ([]shared.RuleSet, error) { | |
client := plugin.NewClient(&plugin.ClientConfig{ | ||
HandshakeConfig: shared.Handshake, | ||
Plugins: shared.PluginMap, | ||
Cmd: exec.Command("sh", "-c", value), | ||
Cmd: exec.Command(value), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [memo] It works by directly passing an executable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember now that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand the flags such that they are passed as arguments to the command, instead of simply passing Naive approach would be to split on spaces, but that would break in the case of the flags containing quoted arguments containing spaces. Writing a quick parser to support this shouldn't be too difficult. eg: Currently, My suggestion would be to use Of course, this is very naive, and wouldnt support any fancy expansions that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your nice suggestion. As I'm not a Windows user, I've been waiting for others' feedbacks, like #144 (comment). These would be used for considering this fix's priority because I have no enough time. |
||
AllowedProtocols: []plugin.Protocol{ | ||
plugin.ProtocolGRPC, | ||
}, | ||
|
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.
[memo] It enables it to properly show an error message when
err
ends up without a new line.