Skip to content

Feature: Repository name filter#88

Merged
Wwwsylvia merged 5 commits intoAzure:mainfrom
DataHow:main
Feb 15, 2022
Merged

Feature: Repository name filter#88
Wwwsylvia merged 5 commits intoAzure:mainfrom
DataHow:main

Conversation

@butzist
Copy link
Contributor

@butzist butzist commented Feb 10, 2022

Purpose of the PR

  • Allow specifying a regex for matching multiple/all repos in a filter rule

Fixes #

@ghost
Copy link

ghost commented Feb 10, 2022

CLA assistant check
All CLA requirements met.

@butzist butzist changed the title Featire: Match multiple repo names Feature: Match multiple repo names Feb 10, 2022
@butzist butzist changed the title Feature: Match multiple repo names Feature: Repository name filter Feb 10, 2022
@butzist
Copy link
Contributor Author

butzist commented Feb 10, 2022

What's the issue with the build? Anything on my side?

@butzist butzist force-pushed the main branch 2 times, most recently from 17b374d to dd9a2c0 Compare February 10, 2022 16:18
@Wwwsylvia
Copy link
Contributor

What's the issue with the build? Anything on my side?

Thanks @butzist for contributing to this repo! The build was broken due to #89 and we've fixed it. You may want to rebase if it still not working.

@butzist
Copy link
Contributor Author

butzist commented Feb 13, 2022

@Wwwsylvia @gildardogmsft Good morning, it's ready for re-review.

@Wwwsylvia
Copy link
Contributor

Thanks @butzist.
Another thing I noticed is that the tag regex allows partial match (@gildardogmsft Is this by design?), while the repo regex does not. Would this confuse users? Should we keep them consistent? I'm not very sure which way users prefer though.

@butzist
Copy link
Contributor Author

butzist commented Feb 14, 2022

@Wwwsylvia The repo regex needs to match the complete name, to mostly perserve backward compatibility and avoid matching too many repos by accident. I left the tag to be partially matching, for the same reason - backwards compatibility. If you want to break compatibility, please do this in a separate PR.

Copy link
Contributor

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

LGTM

@Wwwsylvia
Copy link
Contributor

Thanks @butzist for the clarification! That makes sense.

@butzist
Copy link
Contributor Author

butzist commented Feb 14, 2022

Thanks for the review, @Wwwsylvia

@Wwwsylvia Wwwsylvia merged commit 57b9e96 into Azure:main Feb 15, 2022
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.

3 participants