-
Notifications
You must be signed in to change notification settings - Fork 220
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
handle "run" command in the skip/only setting #634
handle "run" command in the skip/only setting #634
Conversation
4c743cf
to
f4d8b25
Compare
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.
I like the results. It would be nice to have integrity tests (within testdata/
folder) but it looks great.
A few nitpicks related to naming and argument passing.
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.
I've tested the PR with the following config:
skip:
skip:
- run: test -z $VAR
commands:
echo:
run: echo "should not be printed - $VAR"
VAR=1 lefthook run skip -v
But it always prints VAR whenever it is set or not.
I think the problem is that the arguments are passes as raw args to a cmd. Maybe it makes sense to wrap the commandLine with sh -c
the way we do it in exec_unix.go
.
The only concern I have regarding this is that on Windows the feature may not work. But anyway $VAR is sh syntax, not a system syntax, so it makes sense to me.
} | ||
|
||
var cmd *exec.Cmd | ||
if runtime.GOOS == "windows" { |
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.
@mrexox I added calling the command via sh -c
but as you said, it may not work with Windows
if it does not include WSL, Cygwin, and so on.
So I added checking the OS, but I couldn't check it because we don't have a running test-integrity for windows here
I think there was a reason why it was not added to ci-cd for windows :)
But what do you think about checking the windows
and adding test integrity for Windows to ci-cd?
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.
Let's try it out!
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.
@mrexox, Unfortunately, the `[windows] skip' command was necessary to skip some of the Windows test integrations.
Also, I use PowerShell
for Windows commands because it supports more complex commands (e.g. with if
).
We can avoid skipping some test-integrities
, but it requires adapting those tests, and it is difficult without a Windows machine.
The example of skipping test here
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.
but we checked the skip run
for Windows, and it's working.
I added the separated file skip_run_windows
for it
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.
Alright, I think this is acceptable for now. Thank you for digging into testscript, I appreciate it. I will try to figure out how to make it work for Windows when I have a chance to work on a Windows machine.
ee3cd87
to
6eaa1c3
Compare
6eaa1c3
to
348e67c
Compare
Closes #598
⚡ Summary
Added the ability to handle "run" commands in "skip" and "only" sections.
There are a few restrictions
We can only put one "run" command in
skip
/only
sections becauseskip/only
parses like amapstructure
. So we can't writeskip
- run: test "${NO_DAST}" -eq 1
- run: npx is-ci
- ref: dev/*
`
In this PR, we can't put the command with pipelines (
&&, &, ||
etc.), like thistest "${NO_DAST}" -eq 1 && test ....
, because this command is executed viaexec.Command(cmdName, cmdArgs...)
, which accepts the first argument as the command and the rest of the argument as the command arguments.Since
only
is the opposite ofskip,
if therun
command returnstrue
for theskip
section, it will skip, but for theonly
section, it must returnfalse.
☑️ Checklist