feat: Filter copr_build jobs based on paths and files changed

[LecrisUT]

TODO:

• Skip the check if /packit command is used
• Actually check the files changed
• Write new tests or update the old ones to cover new functionality.
• Update or write new documentation in packit/packit.dev.

Depends-on: packit/ogr#921

Fixes packit/packit#1997
Fixes #2006

RELEASE NOTES BEGIN

copr_build jobs are triggered only if there are changed files under the paths field

RELEASE NOTES END

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/d8df73178934488f99f72ae42d7d9818

❌ pre-commit FAILURE in 1m 48s
❌ packit-service-tests FAILURE in 3m 31s

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/ccd7ecf77cb44d22b2ccb47800d4a1e9

✔️ pre-commit SUCCESS in 1m 52s
❌ packit-service-tests FAILURE in 3m 22s

[softwarefactory-project-zuul]

This change depends on a change that failed to merge.

Change packit/ogr#921 is needed.

[LecrisUT]

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?

[lbarcziova]

like the Fedora rebuild commits?

those wouldn't be happening in upstream, so I don't think we have to think about that, or am I missing something?

[LecrisUT]

I mean if they want to have that behaviour. I occasionally do that ¹, but if it's documented that it is or it is not supported, than that's fine as well.

Footnotes

1. https://github.com/LecrisUT/FedoraRPM-atuin/commit/99555ccec9f6ed94fe474767e3b88f18aa835387 ↩

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/3189a66c4a39486e8f902ac669741fa4

✔️ pre-commit SUCCESS in 1m 54s
❌ packit-service-tests FAILURE in 3m 37s

[lbarcziova]

I briefly looked into the code and I am thinking if it wouldn't be more fitting to have the logic for checking this in this method, which would mean it would work generally, not just for Copr build jobs.

[LecrisUT]

I briefly looked into the code and I am thinking if it wouldn't be more fitting to have the logic for checking this in this method, which would mean it would work generally, not just for Copr build jobs.

Probably, but it's not as extendable as with the Checker, needing another place to be aware of that check are being done. I see that there already are checks like pr_labels_match_configuration. What about extending the Checker design (or rather reimplement a similar/simpler class) or at least aggregate the checks in a list of lambdas?

[lbarcziova]

@LecrisUT can you elaborate more on what you are suggesting? I agree the code I linked could be refactored, but it is something different than the Checker's logic, as that is tight to the handlers, while the checks I linked are meant for actually getting the relevant job configurations to the particular event.

[LecrisUT]

@LecrisUT can you elaborate more on what you are suggesting? I agree the code I linked could be refactored, but it is something different than the Checker's logic, as that is tight to the handlers, while the checks I linked are meant for actually getting the relevant job configurations to the particular event.

Indeed, not using the same Checker, but mainly not having all the checks implemented in the function, i.e. something like

    def get_jobs_matching_event(self) -> list[JobConfig]:
        jobs_matching_trigger = []
        checkers = JobChecker.get_all_checkers()
        for job in self.event.packages_config.get_job_views():
            if all(c.check(job) for c in checkers):
                jobs_matching_trigger.append(job)

The Checker design that I like is being able to just create a class and boom a new check is introduced, but I would design it a bit simpler, like

class JobChecker:
    all_checkers: Final[ClassVar[list[type[JobChecker]]]] = []

    def __init_subclass__(cls):
        # Register all checkers when the class is created
        cls.all_checkers.append(cls)

    @classmethod
    def get_all_checkers(cls) -> list[JobChecker]:
        return  [checker_cls() for checker_cls in cls.all_checkers]

    def check(job: JobView) -> bool: ...

(Of course with a better interface, constructor, etc.)

[lbarcziova]

@LecrisUT understood, thanks for the explanation! Yes, that sounds like it could improve the readability and extensibility.

[lachmanfrantisek]

Hi @LecrisUT , thanks for all the work so far. Do you think it's real to finish this? (Asking for the Copr team that would hugely benefit from this..) Anything we can help with?

[LecrisUT]

I can try to make some progress this week.

One question to be answered is #2780 (comment). There are a few ways to approach this:

• Use ogr and navigate to the PR and get the files changed from there
• Use the webhook metadata directly
• Teach ogr to give compare patches ¹ and use the before-after that are already in the webhook payload

I am inclined to go with the 3rd option, just because it will come in handy for the future.

Footnotes

1. https://github.com/PyGithub/PyGithub/blob/2d4785dd08374147a8eab866eeb704508558336a/github/Repository.py#L1342 ↩

[lachmanfrantisek]

Nice, thanks @LecrisUT !

I don't have a strong opinion but using webhook might avoid extra API calls (compared to OGR approach). But on the other hand, not sure if this would be same for other git forges. (With OGR, we might be able to find a way more easily.)

[lbarcziova]

My suggestion would be to try to use webhooks directly as it seems the most straightforward (we have also already been having issues with API rate limits).

[LecrisUT]

Where do I get the push event? Otherwise, it seems we still need the ogr changes for the pull-request trigger, the webhook does not have equivalent data.

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/fce7f73ec1cb408481c1aaf0f4056d20

✔️ pre-commit SUCCESS in 1m 56s
❌ packit-service-tests FAILURE in 3m 26s

[betulependule]

Nice work. It looks good to me so far. Do you think you'll be able to make progress with this soon?

[betulependule]

Where do I get the push event? Otherwise, it seems we still need the ogr changes for the pull-request trigger, the webhook does not have equivalent data.

Could you please elaborate what you mean?

[LecrisUT]

Nice work. It looks good to me so far. Do you think you'll be able to make progress with this soon?

Yes, it's just one blocker to figure out and adding the relevant tests

Where do I get the push event? Otherwise, it seems we still need the ogr changes for the pull-request trigger, the webhook does not have equivalent data.

Could you please elaborate what you mean?

Sure, when I was looking at this, I couldn't find the appropriate mixin that can be used with Checker so that we have the Commit class that would now include the list of commits that is added in f57d857. We have GetPagurePullRequestMixin but I couldn't find the equivalent that would do for upstream PRs.

[betulependule]

Sure, when I was looking at this, I couldn't find the appropriate mixin that can be used with Checker so that we have the Commit class that would now include the list of commits that is added in f57d857. We have GetPagurePullRequestMixin but I couldn't find the equivalent that would do for upstream PRs.

Yes, you're right that there are no equivalents for upstream PRs (as far as I can tell). I suppose we'll need a new Mixin class for this, then? @lbarcziova, what do you think? I'm still rather new to this project so I don't understand this part too much just yet 😅.

[betulependule]

I double checked the definitions of GitHub and GitLab definitions of the MR / PR webhook events and you are definitely right that the information related to what files were changed is not present.

Do I understand correctly that your current approach is the following?

1. In parser.py, once a PR / MR webhook event is received, use ogr to get a list of commits (cast as CommitInfo) contained in this PR / MR.
2. Inside the AreFilesChanged class, take the list of commits and aggregate the three lists of files added, modified and removed.

[LecrisUT]

In the current implementation, it does not go through the ogr and with the changes in the parser.py (parse_github_push_event) it would be materializing the webhook data to a more complete python object with the included files changed (but I did not properly test it, the failing CI should give us some reference for where this is connected).

2. Inside the AreFilesChanged class, take the list of commits and aggregate the three lists of files added, modified and removed.

That is what it is supposed to happen (replacing self.copr_build_helper.pull_request_object.changes, that is still from the ogr required change, I will drop it to make it clearer). As soon as we can get the output of parse_github_push_event in this checker class using a mixin, then that would unlock everything that we were missing here.

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/771c89aa6d1d4d67a65c6ac54086de2b

✔️ pre-commit SUCCESS in 1m 45s
❌ packit-service-tests FAILURE in 5m 51s

[betulependule]

How about we create another Mixin class, which will use the existing get_pr_files_diff function 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.

// dictionary representing files diff
files_changed = project.get_pr_files_diff(pr_id=id_from_pr_event) 

[LecrisUT]

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?

[betulependule]

Oh, you're right. In that case using packit/ogr#921 sounds good to me.

[LecrisUT]

Suggestion: Shall we just try-except this part and make it behave as previously until packit/ogr#921 lands? Or we wait for that first?

Thing is I don't know how raise is handled and if it is safe to use.

[LecrisUT]

Oh neat, the depends-on should actually work according to zuul below 🤔. I guess we can test it with that cross-dependency

[centosinfra-prod-github-app]

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/packit/ogr for 921,9f0bec667f0c9634e7a3c8581d8ec3a31987de1e

[centosinfra-prod-github-app]

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/eaa9311c3e1244389d5d17c363931931

✔️ pre-commit SUCCESS in 1m 50s
❌ packit-service-tests FAILURE in 4m 05s