Skip to content
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

Get rid of shell-out and invoke command directly via exec #334

Merged
merged 1 commit into from Oct 16, 2020

Conversation

StupidScience
Copy link
Contributor

Hello.

Please let me know if this is something you expect here.
Or would be better to switch execCommand to something like execCommand(ctx context.Context, mode ExecMode, commandName string, args ...string)?

Related to #47

@@ -62,7 +62,12 @@ const (

func (*Utils) execCommand(ctx context.Context, mode ExecMode, command string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please change the signature to:

 execKubectlCommand(ctx context.Context, mode ExecMode, arg ...string) (string, error)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the signature change we need to pass an args array, so the current string concatenation should be rewritten to array append.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rebase with main.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I squash it or smth?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please squash

@stefanprodan
Copy link
Member

@StupidScience can you give this a try on Windows and see if it works? Thanks

@StupidScience
Copy link
Contributor Author

Squashed. Also I tried to run gotk check -pre on Win10 and it successfully checked kubectl version. @stefanprodan

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @StupidScience 🎖️

@stefanprodan stefanprodan merged commit a0616ac into fluxcd:main Oct 16, 2020
ybelleguic pushed a commit to ybelleguic/flux2 that referenced this pull request Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants