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

feat(package-rules): add matchRepositories / excludeRepositories #23085

Merged
merged 21 commits into from Jul 21, 2023

Conversation

setchy
Copy link
Collaborator

@setchy setchy commented Jul 2, 2023

Changes

Add new matcher for repository patterns

Context

#23017

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@setchy setchy added type:feature Feature (new functionality) core:package-rules Relating to package-rules e.g. matchers labels Jul 2, 2023
@setchy setchy requested a review from rarkins July 2, 2023 11:04
@setchy setchy assigned setchy and unassigned setchy Jul 2, 2023
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

@setchy
Copy link
Collaborator Author

setchy commented Jul 2, 2023

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

We did 😅

When implementing I noticed that the other matchers which support string/regex arrays follow a match*Patterns naming convention.

I'm happy to go either way

Co-authored-by: Rhys Arkins <rhys@arkins.net>
@setchy setchy requested a review from rarkins July 4, 2023 10:28
lib/util/package-rules/repository-patterns.spec.ts Outdated Show resolved Hide resolved
lib/util/package-rules/repository-patterns.ts Outdated Show resolved Hide resolved
@setchy setchy requested a review from secustor July 4, 2023 11:15
secustor
secustor previously approved these changes Jul 4, 2023
rarkins
rarkins previously approved these changes Jul 4, 2023
@secustor secustor added this pull request to the merge queue Jul 4, 2023
@secustor secustor removed this pull request from the merge queue due to a manual request Jul 4, 2023
@secustor
Copy link
Collaborator

secustor commented Jul 4, 2023

Features are on hold till v36 is released.

lib/util/package-rules/repository-patterns.ts Outdated Show resolved Hide resolved
lib/util/package-rules/repository-patterns.ts Outdated Show resolved Hide resolved
lib/util/package-rules/repository-patterns.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Jul 4, 2023

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

I like this idea, this would reduce the need for multiple matchers. we could do Minimatch by default and regex when starting and ending with slash.

@rarkins
Copy link
Collaborator

rarkins commented Jul 4, 2023

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

I like this idea, this would reduce the need for multiple matchers. we could do Minimatch by default and regex when starting and ending with slash.

@setchy can you make this change?

  • regex if starting/ending in /
  • minimatch otherwise (which supports exact matches too)

@secustor
Copy link
Collaborator

secustor commented Jul 4, 2023

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

I like this idea, this would reduce the need for multiple matchers. we could do Minimatch by default and regex when starting and ending with slash.

@setchy can you make this change?

* regex if starting/ending in `/`

* minimatch otherwise (which supports exact matches too)

I would rather do this in separate PR changes this for all matchers.
If a single matcher has a different syntax, it will be confusing for users.

@setchy
Copy link
Collaborator Author

setchy commented Jul 4, 2023

I thought we talked about matchRepositories allowing for full literal match or /.../ for regex. But do you think this is too inconsistent with other fields?

I like this idea, this would reduce the need for multiple matchers. we could do Minimatch by default and regex when starting and ending with slash.

@setchy can you make this change?

* regex if starting/ending in `/`

* minimatch otherwise (which supports exact matches too)

I would rather do this in separate PR changes this for all matchers. If a single matcher has a different syntax, it will be confusing for users.

Shall we raise a new issue / feature request. Add me as an assignee and I'll work on it as a fast follow this week.

secustor
secustor previously approved these changes Jul 8, 2023
@secustor secustor enabled auto-merge July 8, 2023 07:11
@rarkins
Copy link
Collaborator

rarkins commented Jul 8, 2023

@CodiumAI-Agent

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Adding new matcher for repository patterns
  • 🔍 Description and title: Yes
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • Minimal and focused: Yes, the PR is focused on adding a new feature of matching repositories and all changes are relevant to this feature.
  • 🔒 Security concerns: No, the PR does not introduce any obvious security concerns. The new feature only involves matching repository names, which should not pose any security risks.

PR Feedback

  • 💡 General PR suggestions: The PR is well-structured and follows good practices. The code is clean, and the new feature is well tested. The documentation has been updated to reflect the new changes, which is commendable.

  • 🤖 Code suggestions:

    • relevant file: lib/util/package-rules/repositories.ts
      suggestion content: Consider adding more specific error handling for the isRegexAndMatches function. Currently, it catches any error and assumes it's due to an invalid regex. However, other errors could potentially occur. It would be beneficial to differentiate between an invalid regex error and other types of errors. [medium]

    • relevant file: lib/util/package-rules/repositories.ts
      suggestion content: In the isRegexAndMatches function, consider adding a comment explaining why the pattern is being massaged before being passed to the regEx function. This would improve code readability and maintainability. [medium]

    • relevant file: lib/util/package-rules/repositories.spec.ts
      suggestion content: Consider adding more edge cases to your tests, such as testing with empty strings, strings with special characters, and so on. This will ensure the robustness of your code. [medium]

    • relevant file: lib/config/types.ts
      suggestion content: Consider adding comments to describe the purpose of the new fields matchRepositories and excludeRepositories in the PackageRule interface. This will improve code readability. [medium]

How to use

Tag me in a comment '@CodiumAI-Agent' to ask for a new review after you update the PR.
You can also tag me and ask any question, for example '@CodiumAI-Agent is the PR ready for merge?'

@rarkins rarkins dismissed stale reviews from secustor and themself via c1eff36 July 8, 2023 09:49
@rarkins rarkins requested a review from secustor July 8, 2023 09:59
lib/util/package-rules/match.ts Show resolved Hide resolved
docs/usage/configuration-options.md Show resolved Hide resolved
lib/util/package-rules/repositories.ts Show resolved Hide resolved
auto-merge was automatically disabled July 8, 2023 21:08

Head branch was pushed to by a user without write access

@setchy setchy requested review from viceice and rarkins July 8, 2023 21:20
@setchy
Copy link
Collaborator Author

setchy commented Jul 20, 2023

@rarkins @viceice - anything further from your end re this? Seems general agreement to add minimatch caching separately

@rarkins
Copy link
Collaborator

rarkins commented Jul 20, 2023

Let's add the caching separately

@setchy
Copy link
Collaborator Author

setchy commented Jul 20, 2023

Raised #23491 to track the minimatch caching feature request

@rarkins rarkins enabled auto-merge July 21, 2023 04:21
@rarkins rarkins added this pull request to the merge queue Jul 21, 2023
Merged via the queue into renovatebot:main with commit c85932d Jul 21, 2023
36 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 36.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:package-rules Relating to package-rules e.g. matchers type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants