Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

feat: skip_path option in watch #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

voluntadpear
Copy link

@voluntadpear voluntadpear commented Mar 28, 2022

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 bmatcuk/doublestar#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.

Add support for skip_path option

Add tests

Update README
@@ -154,6 +154,10 @@ Declare a list of
- assets/images/email
config:
trigger: email-deploy
- path: services/webhooks
skip_path: services/webhooks/*.md
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what would be the best name for this tbh

@voluntadpear
Copy link
Author

voluntadpear commented Apr 8, 2022

@xzyfer could you take a look at this please? 🙂 TIA!

@xzyfer
Copy link
Contributor

xzyfer commented Apr 9, 2022

@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 xzyfer closed this Apr 9, 2022
@xzyfer xzyfer reopened this Apr 9, 2022
@codecov-commenter
Copy link

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%) ⬇️
plugin.go 88.88% <100.00%> (-4.34%) ⬇️

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.

@voluntadpear
Copy link
Author

voluntadpear commented Apr 24, 2022

@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 gobwas/glob#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 (vercel/turborepo#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?

@adikari
Copy link
Owner

adikari commented Nov 9, 2022

@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

@heidimhurst
Copy link

heidimhurst commented Dec 8, 2022

Just to offer some encouragement - this would be really useful to have! 🙏

@AppliNH
Copy link

AppliNH commented Dec 15, 2023

Hi! Any news on this? We'd greatly need it, let me know if I can contribute to anything in order to speed up the integration

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants