-
Notifications
You must be signed in to change notification settings - Fork 56
Support wildcards in COPR allowed list #2907
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @evgeni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to Packit's COPR integration by allowing the use of wildcards in the allowed list for forge projects. This change provides greater flexibility for users to define broader patterns for permitted projects, simplifying configuration and management, especially for large organizations or projects with consistent naming conventions. The update ensures that the system can now match project names against patterns like Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for wildcards in the COPR allowed list, which is a great enhancement. The implementation looks correct, and the added tests cover the new functionality well.
I've added a couple of suggestions to improve the robustness of the code by handling cases where the allowed list might be None, and to add a corresponding test case.
Overall, this is a solid contribution.
| ) | ||
| allowed_projects = copr_project["packit_forge_projects_allowed"] | ||
| allowed = self.forge_project in allowed_projects | ||
| allowed = any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allowed_projects variable could be None if the packit_forge_projects_allowed field is not set in the Copr project settings. This would cause an AttributeError when splitlines() is called. To make this more robust, you can handle the None case.
| allowed = any( | |
| allowed = any(fnmatch.fnmatch(self.forge_project, pattern) for pattern in (allowed_projects or "").splitlines()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the API will ever return None here. And the old code wasn't accounting for that either (in doesn't work on None).
| pytest.param( | ||
| JobConfig( | ||
| type=JobType.copr_build, | ||
| trigger=JobConfigTriggerType.pull_request, | ||
| packages={ | ||
| "package": CommonPackageConfig( | ||
| owner="the-owner", | ||
| project="the-project", | ||
| ), | ||
| }, | ||
| ), | ||
| "something/different\ngit.instance.io/the/example/namespace/*", | ||
| True, | ||
| id="wildcard-more-values", | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve test coverage and ensure robustness, please consider adding a test case for when packit_forge_projects_allowed is None. This will verify that the application handles this edge case gracefully without raising an exception.
You could add a parameter like this:
pytest.param(
JobConfig(
type=JobType.copr_build,
trigger=JobConfigTriggerType.pull_request,
packages={
"package": CommonPackageConfig(
owner="the-owner",
project="the-project",
),
},
),
None,
False,
id="none-allowed-list",
),|
Build succeeded. ✔️ packit-service-tests SUCCESS in 19s (non-voting) |
|
Build succeeded. ✔️ packit-service-tests SUCCESS in 19s (non-voting) |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 46s |
lbarcziova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot for the contribution!
| allowed = self.forge_project in allowed_projects | ||
| allowed = any( | ||
| fnmatch.fnmatch(self.forge_project, pattern) | ||
| for pattern in allowed_projects.splitlines() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are receiving list here, so the splitlines() shouldn't be here:
>>> r = c.project_proxy.get("lbarczio", "test-repo")
>>> r["packit_forge_projects_allowed"]
"['github.com/the-project/*', 'github./com/the-project/the-namespace']"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid it's worse than that…
>>> config = {'copr_url': 'https://copr.fedorainfracloud.org'}
>>> c = Client(config=config)
>>> r = c.project_proxy.get("lbarczio", "test-repo")
>>> type(r["packit_forge_projects_allowed"])
<class 'str'>
>>> r["packit_forge_projects_allowed"]
"['github.com/the-project/*', 'github./com/the-project/the-namespace']"
It's a string that looks like a list!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that's with Python 3.13 and copr 2.5 from PyPI, I have not tested any other combos)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/search?q=repo%3Afedora-copr%2Fcopr%20packit_forge_projects_allowed&type=code hints it should be whitespace separated (so the splitlines was technically wrong), but I don't understand why we're getting a stringified list here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, good catch, let me have a better look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no rush on my side, let's do it properly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once fedora-copr/copr#4092 is released, we can proceed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had copr release recently so that would take a while... We could do hotfix for this after new year (but there's high chance I'll forget so in that case please ping me :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem, I think we can wait for the normal release
| ), | ||
| }, | ||
| ), | ||
| "something/different\ngit.instance.io/the/example/namespace/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a quick check I think we have here the test values incorrect as well, all these should be lists, should I have a look or could you cover it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can totally fix these up as I go, but first we need to figure out what the real API response is.
Schema incorrectly defined field as String instead of List(String), causing Flask-RESTX to stringify the list in API responses. Context: packit/packit-service#2907 (comment) Assisted-by: Cursor(Claude)
Schema incorrectly defined field as String instead of List(String), causing Flask-RESTX to stringify the list in API responses. Context: packit/packit-service#2907 (comment) Assisted-by: Cursor(Claude)
TODO:
packit/packit.dev.Fixes #2906
Related to fedora-copr/copr#3804
Merge before/after
RELEASE NOTES BEGIN
Packit now supports wildcards in COPR allow lists
RELEASE NOTES END