Retrigger downstream koji builds #1586

majamassarini · 2022-07-18T13:40:59Z

Through a Pagure comment in a PR

Merge after packit/packit-service-fedmsg#70

RELEASE NOTES BEGIN
A downstream koji-build can now be re-triggered commenting in a PR like this: /packit koji-build.
RELEASE NOTES END

softwarefactory-project-zuul · 2022-07-18T13:55:10Z

Build failed.

✔️ pre-commit SUCCESS in 2m 08s
❌ packit-service-tests FAILURE in 2m 26s
✔️ packit-service-tests-openshift SUCCESS in 13m 52s

softwarefactory-project-zuul · 2022-07-19T09:15:24Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 24s
✔️ packit-service-tests SUCCESS in 2m 53s
✔️ packit-service-tests-openshift SUCCESS in 15m 51s

softwarefactory-project-zuul · 2022-07-19T09:37:14Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 03s
✔️ packit-service-tests SUCCESS in 2m 30s
✔️ packit-service-tests-openshift SUCCESS in 13m 06s

nikromen

I left a one comment about checking whether user who triggers the jobs have write access to the dist-git repository so we don't write duplicated code. Feel free to ignore the other suggestions since they are just nitpicks :D

nikromen · 2022-07-19T12:29:02Z

packit_service/worker/handlers/distgit.py

@@ -461,6 +464,19 @@ def pre_check(self) -> bool:
                        f"configuration: {self.job_config.allowed_committers}."
                    )
                    return False
+        elif self.data.event_type in (PullRequestCommentPagureEvent.__name__,):


I can see that we are doing similar thing here (I am implementing #1541). Can you please move this elif block to the method into JobHandler superclass so I can later use it in CreateBodhiUpdateHandler? Just moving this block of code to separated method in the superclass is enough... I will update this method later to fit the needs of both cases.

(Or if you want, replacing the koji builds part in the stings with some variable should be enough)

When I have fixed it, as Laura wrote maybe I am doing the wrong check now, I will move it in a separate method.

I moved the check in a separate method, but in the same class. I am not really sure you should reuse this check. I think that not every packager (as is for the koji-build) should be able to do a bodhi-update but I may be wrong.

nikromen · 2022-07-19T12:31:12Z

packit_service/worker/events/pagure.py

@@ -90,11 +90,18 @@ def get_dict(self, default_dict: Optional[Dict] = None) -> dict:
        return result

    def get_base_project(self) -> GitProject:
+        if isinstance(self, PullRequestCommentPagureEvent):


personal preference: I would use ternary operator here

maybe I am missing something, but since this is a method of PullRequestCommentPagureEvent, can the else block happen?

Yes you are right! I was thinking of being in a more general PagureEvent...

nikromen · 2022-07-19T12:51:24Z

packit_service/worker/jobs.py

+            self.event, PullRequestCommentPagureEvent
+        ):
+            if (
+                job.type == JobType.koji_build


personal preference: It would be awesome if you named this longer condition so that it's clear to a person what it means without a few extra seconds of thinking (yes, I am a lazy person :D). What about reusing the comment you posted bellow "A koji_build can be retriggered by a Pagure comment in a PR"... so something like can_koji_build_be_retriggered?

nikromen · 2022-07-19T12:51:39Z

packit_service/worker/jobs.py

+                and self.event.job_config_trigger_type
+                == JobConfigTriggerType.pull_request
+            ):
+                "A koji_build can be retriggered by a Pagure comment in a PR"


Suggested change

"A koji_build can be retriggered by a Pagure comment in a PR"

# A koji_build can be retriggered by a Pagure comment in a PR

softwarefactory-project-zuul · 2022-07-19T13:39:42Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 11s
✔️ packit-service-tests SUCCESS in 2m 21s
✔️ packit-service-tests-openshift SUCCESS in 12m 48s

lbarcziova · 2022-07-20T06:17:09Z

packit_service/worker/handlers/distgit.py

+            logger.debug(
+                f"Triggering downstream koji build through comment by: {commenter}"
+            )
+            if commenter not in self.job_config.allowed_committers:


since this is an explicit comment, I think the commenter doesn't need to be allowed in the project configuration but only has a write access to the dist-git repo...but maybe we can discuss it on standup

Ok, so if I understand it correctly: re-triggering a koji-build through a comment should be allowed to more developers.
Some of them, which may be not allowed to do a koji-build through propose-downstream are instead allowed to re-trigger someone else propose-downstream koji-builds, right?

my thinking was that with the configurable allowed_committers and allowed_pr_authors we wanted to preserve the proven packager workflow, but if someone explicitly creates a comment to retrigger the build, it doesn't make that much sense to me to require this user to be allowed in the configuration...@lachmanfrantisek what do you think? should we discuss this in a meeting?

the conclusion from the architecture discussion: we will only require the user who retriggers the koji build by a dist-git comment to be a packager

lbarcziova · 2022-07-20T06:20:03Z

packit_service/worker/handlers/distgit.py

@@ -479,9 +495,14 @@ def run(self) -> TaskResults:
            self.job_config,
            downstream_local_project=self.local_project,
        )
+        branch = (
+            self.project.get_pr(self.data.pr_id).source_branch


we are interested in the branch the changes were merged into, so I think this should be the target_branch

Ok, thanks. I was not sure. Just a thing I do not understand: if I do checkout the target_branch I think I do not have all the commits in the PR and thus I perform a koji-build with not all commits. Am I wrong or this is ok for the koji-build we want?

I think the target_branch should be up to date

packit_service/worker/jobs.py

lbarcziova · 2022-07-20T06:33:46Z

packit_service/worker/events/pagure.py

@@ -90,11 +90,18 @@ def get_dict(self, default_dict: Optional[Dict] = None) -> dict:
        return result

    def get_base_project(self) -> GitProject:
+        if isinstance(self, PullRequestCommentPagureEvent):


maybe I am missing something, but since this is a method of PullRequestCommentPagureEvent, can the else block happen?

softwarefactory-project-zuul · 2022-07-20T08:46:36Z

Build succeeded.

✔️ pre-commit SUCCESS in 3m 08s
✔️ packit-service-tests SUCCESS in 2m 20s
✔️ packit-service-tests-openshift SUCCESS in 16m 45s

softwarefactory-project-zuul · 2022-07-21T13:32:54Z

Build succeeded.

✔️ pre-commit SUCCESS in 1m 59s
✔️ packit-service-tests SUCCESS in 2m 17s
✔️ packit-service-tests-openshift SUCCESS in 13m 12s

softwarefactory-project-zuul · 2022-07-22T12:29:10Z

Build succeeded.

✔️ pre-commit SUCCESS in 3m 50s
✔️ packit-service-tests SUCCESS in 3m 40s
✔️ packit-service-tests-openshift SUCCESS in 17m 38s

lachmanfrantisek

Well done! Thanks!

packit_service/worker/handlers/distgit.py

lachmanfrantisek · 2022-07-22T12:46:26Z

packit_service/worker/jobs.py

+    def allow_non_matching_events(self):
+        """Do explicit exceptions for non matching event/jobs triggers
+
+        Returns:
+            list of jobs
+        """


I am still wondering why is this needed and if we can't fix this elsewhere. This is because of the commit vs pull_request trigger, isn't it?

TBH, I was a bit confused by the method name and docstring. Could you please rename it to something like check_manual_mapping/check_explicit_matching/.. or something like that? (From the name, I thought we are allowing any combination...;) And then, I've seen the word exception and was confused completely..;)

Yes this is because of the commit vs pull_request trigger.
I may be wrong but I don't see how to allow this without this hack.
But we could probably ask to the users to put in their configuration a job with trigger pull_request for the koji_build if they want to trigger it through a comment in a pull_request.
I copied the config file from osbuilder project and they do not have such a job in their config, they only have a koji_build job triggered by a commitand I wanted my code to be able to trigger a build with their configuration.
But I can remove this lines if we agree that a change in the config file should be needed.

No, I don't think we should ask users to add another job. (It's same as with the propose-downstream where nothing is needed to be able to retrigger the job by an issue comment.)

We can leave it as is and try to refactor it later.

Through Pagure comments on PR

softwarefactory-project-zuul · 2022-07-25T08:14:07Z

Build succeeded.

✔️ pre-commit SUCCESS in 2m 24s
✔️ packit-service-tests SUCCESS in 2m 07s
✔️ packit-service-tests-openshift SUCCESS in 13m 30s

lbarcziova

thanks! let's test this on stage

lbarcziova · 2022-07-26T06:29:34Z

packit_service/worker/handlers/distgit.py

+            logger.debug(
+                f"Triggering downstream koji build through comment by: {commenter}"
+            )
+            if not self.is_packager(commenter):


in this case, it could be nice to notify user about this, but we can discuss if and how we want to do it (PR comment?) as a follow-up and be also consistent with retriggering bodhi updates

softwarefactory-project-zuul · 2022-07-26T07:59:32Z

Build succeeded (gate pipeline).

✔️ pre-commit SUCCESS in 2m 02s

majamassarini force-pushed the fix/packit-service/1540 branch from 3d798a1 to 79d5003 Compare July 19, 2022 08:59

majamassarini force-pushed the fix/packit-service/1540 branch from 79d5003 to 7d031e2 Compare July 19, 2022 09:23

nikromen reviewed Jul 19, 2022

View reviewed changes

majamassarini force-pushed the fix/packit-service/1540 branch from 7d031e2 to ec89652 Compare July 19, 2022 13:26

lbarcziova reviewed Jul 20, 2022

View reviewed changes

majamassarini force-pushed the fix/packit-service/1540 branch from ec89652 to 5eefafd Compare July 20, 2022 08:29

majamassarini marked this pull request as ready for review July 20, 2022 08:47

majamassarini force-pushed the fix/packit-service/1540 branch from 5eefafd to 476081c Compare July 21, 2022 13:19

majamassarini force-pushed the fix/packit-service/1540 branch from 476081c to 445f515 Compare July 22, 2022 12:10

lachmanfrantisek reviewed Jul 22, 2022

View reviewed changes

Retrigger downstream koji builds

5bb916f

Through Pagure comments on PR

majamassarini force-pushed the fix/packit-service/1540 branch from 445f515 to 5bb916f Compare July 25, 2022 08:00

lbarcziova approved these changes Jul 26, 2022

View reviewed changes

majamassarini added the mergeit When set, zuul wil gate and merge the PR. label Jul 26, 2022

softwarefactory-project-zuul bot merged commit 1194bc5 into packit:main Jul 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrigger downstream koji builds #1586

Retrigger downstream koji builds #1586

majamassarini commented Jul 18, 2022 •

edited

Loading

softwarefactory-project-zuul bot commented Jul 18, 2022

softwarefactory-project-zuul bot commented Jul 19, 2022

softwarefactory-project-zuul bot commented Jul 19, 2022

nikromen left a comment

nikromen Jul 19, 2022 •

edited

Loading

majamassarini Jul 20, 2022

majamassarini Jul 22, 2022

nikromen Jul 19, 2022

lbarcziova Jul 20, 2022

majamassarini Jul 20, 2022

nikromen Jul 19, 2022

nikromen Jul 19, 2022

softwarefactory-project-zuul bot commented Jul 19, 2022

lbarcziova Jul 20, 2022

majamassarini Jul 20, 2022

lbarcziova Jul 20, 2022

lbarcziova Jul 21, 2022

lbarcziova Jul 20, 2022

majamassarini Jul 20, 2022

lbarcziova Jul 20, 2022

lbarcziova Jul 20, 2022

softwarefactory-project-zuul bot commented Jul 20, 2022

softwarefactory-project-zuul bot commented Jul 21, 2022

softwarefactory-project-zuul bot commented Jul 22, 2022

lachmanfrantisek left a comment

lachmanfrantisek Jul 22, 2022

majamassarini Jul 22, 2022 •

edited

Loading

lachmanfrantisek Jul 22, 2022

softwarefactory-project-zuul bot commented Jul 25, 2022

lbarcziova left a comment

lbarcziova Jul 26, 2022

softwarefactory-project-zuul bot commented Jul 26, 2022

	"A koji_build can be retriggered by a Pagure comment in a PR"
	# A koji_build can be retriggered by a Pagure comment in a PR

Retrigger downstream koji builds #1586

Retrigger downstream koji builds #1586

Conversation

majamassarini commented Jul 18, 2022 • edited Loading

softwarefactory-project-zuul bot commented Jul 18, 2022

softwarefactory-project-zuul bot commented Jul 19, 2022

softwarefactory-project-zuul bot commented Jul 19, 2022

nikromen left a comment

Choose a reason for hiding this comment

nikromen Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jul 20, 2022

softwarefactory-project-zuul bot commented Jul 21, 2022

softwarefactory-project-zuul bot commented Jul 22, 2022

lachmanfrantisek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majamassarini Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jul 25, 2022

lbarcziova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jul 26, 2022

majamassarini commented Jul 18, 2022 •

edited

Loading

nikromen Jul 19, 2022 •

edited

Loading

majamassarini Jul 22, 2022 •

edited

Loading