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

handle "run" command in the skip/only setting #634

Merged
merged 10 commits into from
Mar 13, 2024

Conversation

prog-supdex
Copy link
Contributor

Closes #598

⚡ Summary

Added the ability to handle "run" commands in "skip" and "only" sections.

There are a few restrictions

  1. We can only put one "run" command in skip/only sections because skip/only parses like a mapstructure. So we can't write
    skip
    - run: test "${NO_DAST}" -eq 1
    - run: npx is-ci
    - ref: dev/*
    `

  2. In this PR, we can't put the command with pipelines (&&, &, || etc.), like this test "${NO_DAST}" -eq 1 && test ...., because this command is executed via exec.Command(cmdName, cmdArgs...), which accepts the first argument as the command and the rest of the argument as the command arguments.

  3. Since only is the opposite of skip, if the run command returns true for the skip section, it will skip, but for the only section, it must return false.

☑️ Checklist

  • Check locally
  • Add tests
  • Add documentation

Copy link
Member

@mrexox mrexox left a 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.

internal/config/skip.go Outdated Show resolved Hide resolved
internal/config/skip.go Outdated Show resolved Hide resolved
internal/config/script.go Outdated Show resolved Hide resolved
internal/config/skip.go Outdated Show resolved Hide resolved
Copy link
Member

@mrexox mrexox left a 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" {
Copy link
Contributor Author

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?

Copy link
Member

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!

Copy link
Contributor Author

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

Copy link
Contributor Author

@prog-supdex prog-supdex Mar 12, 2024

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

Copy link
Member

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.

@prog-supdex prog-supdex requested a review from mrexox March 11, 2024 15:11
@prog-supdex prog-supdex force-pushed the skip_only_conditions branch 12 times, most recently from ee3cd87 to 6eaa1c3 Compare March 12, 2024 12:46
@prog-supdex prog-supdex force-pushed the skip_only_conditions branch from 6eaa1c3 to 348e67c Compare March 12, 2024 12:58
@mrexox mrexox merged commit e9ca955 into evilmartians:master Mar 13, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using conditions for Skip and Only
2 participants