-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: rework setupShellCommand #930
Conversation
7a49419
to
b0766d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #930 +/- ##
==========================================
+ Coverage 49.27% 56.81% +7.54%
==========================================
Files 23 28 +5
Lines 2401 4161 +1760
==========================================
+ Hits 1183 2364 +1181
- Misses 1090 1590 +500
- Partials 128 207 +79
Continue to review full report at Codecov.
|
b0766d6
to
e74b260
Compare
* move all logic to separate function so we can test that later * split `step.Shell` and `step.WorkingDirectory` setup into own funcs * general cleanup of function * use `ActPath` to not collide with checked out repository * use `shellquote.Split()` instead of `strings.Fields()` for better command split * replace single string concat with `fmt` Signed-off-by: hackercat <me@hackerc.at>
e74b260
to
bd70028
Compare
Signed-off-by: hackercat <me@hackerc.at>
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.
Looks fine with just a question from my side
step.Shell
andstep.WorkingDirectory
setup into own funcsActPath
to not collide with checked out repository (also avoids issues if path contains spaces)shellquote.Split()
instead ofstrings.Fields()
for better command splitfmt
Fixes #884
Fixes #865
Signed-off-by: hackercat me@hackerc.at