upsidr / merge-gatekeeper

Get better merge control
MIT License
85 stars 14 forks source link

Add regex support for ignored jobs #50

Open Jrc356 opened 1 year ago

Jrc356 commented 1 year ago

It would be cool if we could support regex matching on ignored jobs. This way we can specify something like deploy-* and have it ignore all the deploy jobs

rytswd commented 1 year ago

Thanks @Jrc356, I can see this being quite a useful feature!

However, I'm a bit unsure whether we should offer the straight regex support in the current ignored field, or we should create a separate ignoredRegex field. If we were to work this out using the existing field, this could be a breaking change (although it would be rather unlikely).

As GitHub Action input doesn't allow other data types such as lists, if we were to support multiple regex inputs, we would need some delimiter in place. The current implementation of using comma may be quite limiting and annoying, though. For instance, jobs using matrix strategy would have test name (a, x) type of format, and csv handling happening separately could well break the regex behaviour. Perhaps we could use a line break as a delimiter instead...?

Do you have some other example use cases for regex, and what you would want to achieve?

hlts2 commented 1 year ago

Thanks for the suggestion! I think it is a very useful feature. 👍

However. I have a few concerns. If we also support regular expressions in ignored fields, there is a risk that this could result in unexpected bugs when existing users use special characters (. or *) in their job names. So if we add it, we may add a regular expression field, etc.

Jrc356 commented 1 year ago

@hlts2 @rytswd yeah that makes sense to me. A new ignoredRegex field or a regex: true|false flag that enables passing regex to the ignore field (sounds unnecessarily complicated over an ignoreRegex field).

As GitHub Action input doesn't allow other data types such as lists, if we were to support multiple regex inputs, we would need some delimiter in place. The current implementation of using comma may be quite limiting and annoying, though. For instance, jobs using matrix strategy would have test name (a, x) type of format, and csv handling happening separately could well break the regex behaviour. Perhaps we could use a line break as a delimiter instead...?

Assuming a new ignoredRegex field is created, I would agree that newline makes a good delimiter here. With the changes in #45, ignored can also handle newlines which keeps things consistent

rytswd commented 1 year ago

@Jrc356 Sorry to loop back late - a separate flag sounds like a better idea than having a list of jobs in regex form 🤩 I'm rather thinking out loud, but we may want to consider supporting non-regex, simplified wildcard support instead - regex does provide a lot of flexibility, but we may not need that much flexibility. Simple * wildcard support would be already beneficial for most use cases IMO. What do you think? 🤔

Regarding the newline handling, I think the current implementation of ignored field handling is slightly different, though. It expects a CSV input, which means:

👍 GOOD

ignored: |
  a,
  b,
  c

🙅 NOT GOOD (as of writing)

ignored: |
  a
  b
  c

Along with adding a flag to support, we may want to extend the support for newline as a delimiter, without commas. With that, it would be easier to clearer for users to see each line as regex / wildcard input, with no extra complication.

Jrc356 commented 1 year ago

@rytswd no worries 😊

@Jrc356 Sorry to loop back late - a separate flag sounds like a better idea than having a list of jobs in regex form 🤩 I'm rather thinking out loud, but we may want to consider supporting non-regex, simplified wildcard support instead - regex does provide a lot of flexibility, but we may not need that much flexibility. Simple * wildcard support would be already beneficial for most use cases IMO. What do you think? 🤔

A simplified wildcard could work. I imagine there might be an edge case for supporting regex such as numbers, example:

ignored: |
  my_job[0-4] #skip the jobs 0 to 4 but watch all others

but I imagine this use case is few and far between. That being said, I think wildcard support would solve most cases.


Along with adding a flag to support, we may want to extend the support for newline as a delimiter, without commas. With that, it would be easier to clearer for users to see each line as regex / wildcard input, with no extra complication.

+1 on switching the delimiter to newline instead of comma but I imagine this would be breaking? Or perhaps you were thinking of something else here and I'm misinterpreting?

rytswd commented 1 year ago

@Jrc356 If we introduce a string based flag and allow the use of csv, wildcard, regex, for example, I think we would be able to start with simple solution first, and add extra support as necessary. The example you provided with my_job[0-4] would be too complicated for wildcard solution, so if it's absolutely necessary, we would probably need to have a regex setup.

As to the newline delimiter, we should be able to support both comma and newline at the same time (at least for when the ignore mode is set to csv). We will need more test cases to cover all the edge cases, but I think we can probably add that without breaking the existing behaviour - I may be missing some points though 🤔

Jrc356 commented 1 year ago

That sounds good to me!

The example you provided with my_job[0-4] would be too complicated for wildcard solution, so if it's absolutely necessary, we would probably need to have a regex setup.

I don't think this would be necessary, just an edge case I could think of. I don't think it would be a common use case though