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

Add regex support for ignored jobs #50

Open
Jrc356 opened this issue Nov 1, 2022 · 7 comments
Open

Add regex support for ignored jobs #50

Jrc356 opened this issue Nov 1, 2022 · 7 comments

Comments

@Jrc356
Copy link
Contributor

Jrc356 commented Nov 1, 2022

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
Copy link
Member

rytswd commented Nov 2, 2022

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
Copy link
Contributor

hlts2 commented Nov 2, 2022

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
Copy link
Contributor Author

Jrc356 commented Nov 2, 2022

@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
Copy link
Member

rytswd commented Nov 8, 2022

@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
Copy link
Contributor Author

Jrc356 commented Nov 8, 2022

@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
Copy link
Member

rytswd commented Nov 8, 2022

@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
Copy link
Contributor Author

Jrc356 commented Nov 14, 2022

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

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

No branches or pull requests

3 participants