[WIP] feat: Filter `copr_build` jobs based on `paths` and files changed

[LecrisUT]

TODO:

• [ ] Skip the check if /packit command is used
• [x] 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: https://github.com/packit/ogr/pull/921

Fixes https://github.com/packit/packit/issues/1997 Fixes https://github.com/packit/packit-service/issues/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[bot]]

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

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

[softwarefactory-project-zuul[bot]]

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

:heavy_check_mark: pre-commit SUCCESS in 1m 52s :x: packit-service-tests FAILURE in 3m 22s

[softwarefactory-project-zuul[bot]]

This change depends on a change that failed to merge.

Change https://github.com/packit/ogr/pull/921 is needed.

[softwarefactory-project-zuul[bot]]

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

:heavy_check_mark: pre-commit SUCCESS in 1m 54s :x: 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 https://github.com/packit/packit-service/pull/2780#discussion_r2111302592. 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 ^1 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.

[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[bot]]

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

:heavy_check_mark: pre-commit SUCCESS in 1m 56s :x: packit-service-tests FAILURE in 3m 26s

[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 https://github.com/packit/packit-service/commit/f57d85790f836fd74afbe61deb7cbe5971bc985c. 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 :sweat_smile:.

[softwarefactory-project-zuul[bot]]

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

:heavy_check_mark: pre-commit SUCCESS in 1m 45s :x: packit-service-tests FAILURE in 5m 51s