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

added regex functionality for allow lists in the analyzer #1357

Merged
merged 11 commits into from
Apr 25, 2024

Conversation

NarekAra
Copy link
Contributor

@NarekAra NarekAra commented Apr 13, 2024

Change Description

Although the allow_list functionality is useful, it is not very practical as regex functionality is not handled here. I added this while still allowing the regular allow list to exist.

Issue reference

This PR fixes issue NONE

Checklist

  • [ x] I have reviewed the contribution guidelines
  • [ x] I have signed the CLA (if required)
  • [ x] My code includes unit tests
  • [ x] All unit tests and lint checks pass locally
  • [ x] My PR contains documentation updates / additions if required

@NarekAra
Copy link
Contributor Author

@microsoft-github-policy-service agree

@omri374
Copy link
Contributor

omri374 commented Apr 14, 2024

Thanks! How would you suggest to differentiate this from the original allow list capabilities? When should users use which one?

@omri374
Copy link
Contributor

omri374 commented Apr 14, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NarekAra
Copy link
Contributor Author

NarekAra commented Apr 14, 2024 via email

@omri374
Copy link
Contributor

omri374 commented Apr 15, 2024

Is it not possible because of regex flags? Or something else?

@omri374
Copy link
Contributor

omri374 commented Apr 15, 2024

If you could give an example where you call the new regrx_allow_list that'd be great.

@NarekAra
Copy link
Contributor Author

NarekAra commented Apr 15, 2024 via email

@omri374
Copy link
Contributor

omri374 commented Apr 16, 2024

If we made the deny list case insensitive, would that support your use case?

@NarekAra
Copy link
Contributor Author

NarekAra commented Apr 16, 2024 via email

@omri374
Copy link
Contributor

omri374 commented Apr 16, 2024

Sorry, I meant allow list :) if we make the existing allow list case insensitive, would that be ok? I would rather not add more parameters or capabilities as it's confusing to users, and I'd rather not remove parameters as it's not backward compatible

@NarekAra
Copy link
Contributor Author

NarekAra commented Apr 16, 2024 via email

@omri374
Copy link
Contributor

omri374 commented Apr 17, 2024

Enhancing the existing capability with a parameters on how to process it sounds like a great improvement!

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Hi, this is a great addition! Left a few comments mainly around being more explicit on what this does and how.

omri374
omri374 previously approved these changes Apr 24, 2024
@omri374
Copy link
Contributor

omri374 commented Apr 24, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374
Copy link
Contributor

omri374 commented Apr 24, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374
Copy link
Contributor

omri374 commented Apr 24, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Thanks! Approved

@omri374 omri374 merged commit 55bfb8f into microsoft:main Apr 25, 2024
31 checks passed
@NarekAra
Copy link
Contributor Author

Great, thanks for reviewing! Indeed, a big improvement :)

@NarekAra NarekAra deleted the allow_list branch July 23, 2024 20:23
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.

2 participants