-
Notifications
You must be signed in to change notification settings - Fork 56
[WIP] feat: Filter copr_build jobs based on paths and files changed
#2780
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,14 @@ | |
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import logging | ||
| from pathlib import Path | ||
|
|
||
| from packit.config import JobConfigTriggerType | ||
|
|
||
| from packit_service.constants import ( | ||
| INTERNAL_TF_BUILDS_AND_TESTS_NOT_ALLOWED, | ||
| ) | ||
| from packit_service.events import gitlab | ||
| from packit_service.events import github, gitlab | ||
| from packit_service.worker.checker.abstract import ( | ||
| ActorChecker, | ||
| Checker, | ||
|
|
@@ -138,3 +141,53 @@ def _pre_check(self) -> bool: | |
| ) | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| class AreFilesChanged(Checker, GetCoprBuildJobHelperForIdMixin, ConfigFromEventMixin): | ||
| """ | ||
| Check if any files under the current package's `paths` field is changed. | ||
| If not, then just skip the current copr build job. | ||
| """ | ||
|
|
||
| def get_files_changed(self) -> list[Path]: | ||
| """ | ||
| Get the list of files changed in the current commit or the current pullrequest | ||
| """ | ||
| # Get the changes object | ||
| if self.job_config.trigger == JobConfigTriggerType.pull_request: | ||
| pr_event = self.data.to_event() | ||
| if not isinstance(pr_event, (github.pr.Action, gitlab.mr.Action)): | ||
| # TODO: What about comments? | ||
| raise NotImplementedError() | ||
| # TODO: How to handle PRs? | ||
| raise NotImplementedError() | ||
| if self.job_config.trigger == JobConfigTriggerType.commit: | ||
| push_event = self.data.to_event() | ||
| if not isinstance(push_event, (github.push.Commit, gitlab.push.Commit)): | ||
| raise NotImplementedError() | ||
| files = set() | ||
| for commit in push_event.commits: | ||
| files |= set(commit["modified"]) | ||
| files |= set(commit["added"]) | ||
| # TODO: Check what the path is relative to | ||
| return [Path(file) for file in files] | ||
| raise NotImplementedError(f"Trigger not supported: {self.job_config.trigger}") | ||
|
|
||
| def pre_check(self) -> bool: | ||
| if self.job_config.trigger == JobConfigTriggerType.release: | ||
| # For releases we don't do any checks | ||
| return True | ||
| # FIXME: This is probably unnecessary | ||
| package_config = self.package_config.get_package_config_for(self.job_config) | ||
|
Comment on lines
+180
to
+181
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think this might be unnecessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about the usage of the method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it seems that the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, worst case this is just a no-op, but if anyone with more experience could chyme in and confirm it quickly then we can resolve this one way or the other. Let's keep this open for a bit. |
||
| # The paths that we need to check for files changed | ||
| paths = package_config["paths"] | ||
| # Early check if the git root was included, in which case we don't need to | ||
| # check the files changed | ||
| # TODO: refine this check when gitignore-like patterns are supported | ||
| if "./" in paths: | ||
| return True | ||
| for changed_file in self.get_files_changed(): | ||
| # Check if any of the files changed are under the paths that are being tracked | ||
| if any(changed_file.is_relative_to(p) for p in paths): | ||
| return True | ||
| return False | ||
|
Comment on lines
+189
to
+193
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, what do we do if we have an empty commit, like the Fedora rebuild commits? Should it be a special case to skip this check?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
those wouldn't be happening in upstream, so I don't think we have to think about that, or am I missing something?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean if they want to have that behaviour. I occasionally do that 1, but if it's documented that it is or it is not supported, than that's fine as well. 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.
How about we create another Mixin class, which will use the existing
get_pr_files_difffunction in ogr to retrieve the files diff? It should return a dictionary, so we would need to process it into a list of files changed.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.
It does not seem to be available for github and gitlab? https://github.com/search?q=repo%3Apackit%2Fogr%20get_pr_files_diff&type=code And I think the internal API would not make it efficient to query that format. Maybe we go through packit/ogr#921?
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.
Oh, you're right. In that case using packit/ogr#921 sounds good to me.