monorepo-diff-buildkite-plugin icon indicating copy to clipboard operation
monorepo-diff-buildkite-plugin copied to clipboard

feat: `skip_path` option in `watch`

Open voluntadpear opened this issue 2 years ago β€’ 7 comments

This PR adds support for specifying a skip_path option that support either a path or a list of paths that shouldn't result in adding the trigger step as part of the dynamically generated pipeline.

Note even in cases where a line in the diff is skipped due to skip_path and there are other paths in the diff that match path but doesn't match skip_path, the step will still be added to the dynamically generated pipeline. i.e. for the step to be effectively ignored, the paths in skip_path should match all of those in the given diff.

Motivation

The motivation for this is the lack of support for negation cases in doublestar (the package used for glob pattern matching) See https://github.com/bmatcuk/doublestar/issues/49.

I had a use case where I needed to avoid changes in certain subdirectories to trigger a step (if there were the only changes in the diff) and I tried to use the following pattern: path: "/main/{*,!(subdir1|subdir2)/**/*}" which should be a valid glob for filtering out subdir1 and subdir2, but didn't work.

With this, I could instead do something like:

- path: "main/"
  skip_path:
    - "main/subdir1"
    - "main/subdir2"

and the result would be the same.

voluntadpear avatar Mar 28 '22 01:03 voluntadpear

@xzyfer could you take a look at this please? πŸ™‚ TIA!

voluntadpear avatar Apr 08 '22 13:04 voluntadpear

@voluntadpear the capability proposed is valuable but a better approach would be to use a more robust globbing library with support for pattern negation i.e. https://github.com/gobwas/glob. That way we can consolidate all the matches in the path field.

xzyfer avatar Apr 09 '22 09:04 xzyfer

Codecov Report

Merging #88 (8e87cc1) into master (bbda0a1) will decrease coverage by 1.07%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   78.48%   77.40%   -1.08%     
==========================================
  Files           4        4              
  Lines         158      177      +19     
==========================================
+ Hits          124      137      +13     
- Misses         22       27       +5     
- Partials       12       13       +1     
Impacted Files Coverage Ξ”
pipeline.go 68.42% <71.42%> (-0.16%) :arrow_down:
plugin.go 88.88% <100.00%> (-4.34%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update bbda0a1...8e87cc1. Read the comment docs.

codecov-commenter avatar Apr 09 '22 10:04 codecov-commenter

@xzyfer I've taken a look at https://github.com/gobwas/glob/ and there's also a similar issue to the one of doublestar, as reported in https://github.com/gobwas/glob/issues/47 negation only works on a character range or single character basis rather than a character group.

Also, it seems it hasn't been actively maintained for a while now.

I've tried something like "/main/{*,!(subdir1|subdir2)/**/*}" and many variations but I couldn't find any way to negate subdir1 and subdir2 as desired for this use case here.

I saw that the turborepo team faced the same issue (https://github.com/vercel/turborepo/issues/445) with gobwas/glob and added a --exclude flag to specify paths to exclude, similar to what I'm proposing in this PR (btw, excludes sounds like an excellent alternative to skip_path), so this seems like a valid alternative.

What do you think?

voluntadpear avatar Apr 24 '22 23:04 voluntadpear

@voluntadpear Sorry for delay. how about we do something as below.

Option 1: add a new property we do something like

- path: 
  - "main/"
  - "!main/subdir1"
  - "!main/subdir2"

Basically allow the path to be either string value of array.

Option 2: add a new property like you suggested but rename it to exclude.

I prefer option1 though

adikari avatar Nov 09 '22 00:11 adikari

Just to offer some encouragement - this would be really useful to have! πŸ™

heidimhurst avatar Dec 08 '22 13:12 heidimhurst