Skip to content
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

ux: merge downstream without triggering build #2186

Closed
2 tasks done
LecrisUT opened this issue Sep 15, 2023 · 13 comments
Closed
2 tasks done

ux: merge downstream without triggering build #2186

LecrisUT opened this issue Sep 15, 2023 · 13 comments
Labels
area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. gain/high This brings a lot of value to (not strictly a lot of) users. impact/low This issue impacts only a few users. kind/feature New feature or a request for enhancement.

Comments

@LecrisUT
Copy link
Contributor

Description

There should be some mechanism to merge a packit PR (with koji_build job enabled) without triggering a koji build and/or use a new/specific tag. This is particularly important on rawhide where otherwise the builds need to be untagged to fix potential dependency issues.

Benefit

No response

Importance

Medium. The packager can always create a new branch in their fork, but it would be a pain to do so.

What is the impacted category (job)?

Fedora release automation

Workaround

  • There is an existing workaround that can be used until this feature is implemented.

Participation

  • I am willing to submit a pull request for this issue. (Packit team is happy to help!)
@LecrisUT LecrisUT added the kind/feature New feature or a request for enhancement. label Sep 15, 2023
@lachmanfrantisek
Copy link
Member

Hi @LecrisUT, what about using labels for that? (E.g. packit-skip-koji-build). (On the other hand, the info should ideally be in commit, not the PR.)

Medium. The packager can always create a new branch in their fork, but it would be a pain to do so.

Good point, one can still recreate the PR.

@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Sep 18, 2023

Also, this should probably skip the build also if allowed_pr_authors is configured, right?

@LecrisUT
Copy link
Contributor Author

Labels should be fine. It might be tricky if not all maintainers have the write privilege to the PR labels, but that's an organization issue for the package.

On the other hand, the info should ideally be in commit, not the PR.

That would be tricky with %autochangelog

@lachmanfrantisek
Copy link
Member

lachmanfrantisek commented Sep 18, 2023

But we can start with the label and see how this works. Will you take a look at that, Cristian? It won't be hard I hope. I can give you some pointers if you want, but you are quite familiar with the codebase as far as I know..;-)

@LecrisUT
Copy link
Contributor Author

I think I could navigate this issue. Probably will go through ogr to get the label. Otherwise it's in packit-service and not packit repo right?

@majamassarini majamassarini added area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. impact/low This issue impacts only a few users. gain/high This brings a lot of value to (not strictly a lot of) users. labels Sep 18, 2023
@majamassarini majamassarini moved this from new to backlog in Packit Kanban Board Sep 18, 2023
@lachmanfrantisek
Copy link
Member

I think I could navigate this issue. Probably will go through ogr to get the label. Otherwise it's in packit-service and not packit repo right?

Yes, it should be in packit-service. Probably as a new checker here:

@staticmethod
def get_checkers() -> Tuple[Type[Checker], ...]:
return (PermissionOnDistgit, HasIssueCommenterRetriggeringPermissions)

@lachmanfrantisek
Copy link
Member

@LecrisUT just a gentle ping. Are you still interested in taking a look into this? (There is a related issue packit/packit#1918 so I would like to know have we stand with this..)

@lachmanfrantisek
Copy link
Member

When thinking about this again, we can:

  • Have the label name configurable.
  • When looking at Packit doesn't perform koji_build when only the release is bumped while using %autorelease packit#1918, it might be handy to support both allowing and disallowing so people can also trigger the build even if the PRs author is not configured. (=> So, have a pair of labels.)
  • I hope labels (aka tags) are ok from the security point of view => labels do not require commit permissions to dist-git repo[0], but:
    • Someone with the admin rights needs to give the person triage permissions.
    • Someone with the commit permissions needs to configure this.

[0]: Actually, the docs speak only about issue labels:
image

@lachmanfrantisek
Copy link
Member

Alternatively, we can use commits for that. (This has already been requested some time ago.)

This might work as follows:

  1. We will react to a new dist-git commit (we wouldn't react to comments itself).
  2. When checking the permissions, we will also go through the respective PR comments backwards (from newest to oldest) and if we find /packit allow-koji-build / /packit disable-koji-build (or some other names) and the comment author has commit rights to the repo, we will use this info instead of the permissions check to decide if the build should be triggered.

Implementation-wise, this might be a bit more complicated than with tags but still very easy thanks to OGR.

To me, the label looks more prominent and well-visible. And both comments and label actions are visible in the PR history:
image

@LecrisUT
Copy link
Contributor Author

Yeah, I think labels will be easier to manage, I'll take a look this coming week. Thanks for the reminder.

  • I hope labels (aka tags) are ok from the security point of view => labels do not require commit permissions to dist-git repo[0]

Should be fine because this should be checked when merging the PR.

Yeah, I can see how triggering build with label can be useful. I don't think it can address packit/packit#1918 because the labels can only be on PRs, and you cannot make an empty commit PR in Pagure. I guess this can be partially resolved if we support non-empty PRs

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Oct 17, 2023

I think I could navigate this issue. Probably will go through ogr to get the label. Otherwise it's in packit-service and not packit repo right?

Yes, it should be in packit-service. Probably as a new checker here:

@staticmethod
def get_checkers() -> Tuple[Type[Checker], ...]:
return (PermissionOnDistgit, HasIssueCommenterRetriggeringPermissions)

Some quick notes and questions looking at the implementation:

  • Does Checker.pre_check() run at every event from Pagure? I shouldn't worry about any persistent configurations due to Checker?
  • As I read it Checker.pre_check() only has value in early-exiting. That would work for skip-build label for this specific issue. But for force-build label there needs to be a hook to overwrite the default checks by packit that check if it should trigger a build. Any clues on where to hook that?
  • In Checker/any mixin, can I get the details of the event? This checks should only be run if the event is merge PR, and not if the event is comment. Currently I am looking at Checker.data.event_dict, but it doesn't seem to have the interface:
    • event_dict["action"] is one of PullRequestAction right? That one does not have a closed or a merged action to check?

@lbarcziova
Copy link
Member

hi @LecrisUT !

Does Checker.pre_check() run at every event from Pagure? I shouldn't worry about any persistent configurations due to Checker?

pre_check runs each time we create a handler (=> match event to some tasks that need to be run based on the configuration), see this

As I read it Checker.pre_check() only has value in early-exiting. That would work for skip-build label for this specific issue. But for force-build label there needs to be a hook to overwrite the default checks by packit that check if it should trigger a build. Any clues on where to hook that?

Not sure about this one, maybe you can explain the problem more, I would say with the right order of the checks we could make it work.

In Checker/any mixin, can I get the details of the event? This checks should only be run if the event is merge PR, and not if the event is comment. Currently I am looking at Checker.data.event_dict, but it doesn't seem to have the interface:

yes, that's right, the data.event_dict will include the information about the event itself. But since with Koji build we react to new dist-git commits, it will be probably easier to get the PR via API and get the info about the labels from there. You can get inspired in how we check the PR author here:

pr_author = self.get_pr_author()

@lbarcziova
Copy link
Member

As I work on #2269, I will probably cover the labels implementation for this issue as well.

softwarefactory-project-zuul bot added a commit that referenced this issue Feb 7, 2024
Conditions on PR labels

TODO:

 docs

Fixes #2269 #2186

RELEASE NOTES BEGIN
We have introduced new configuration options require.label.present and require.label.absent. By configuring these you can specify labels that need to be present or absent on a pull request for Packit to react on such PR.
RELEASE NOTES END

Reviewed-by: František Lachman <[email protected]>
@github-project-automation github-project-automation bot moved this from backlog to done in Packit Kanban Board Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fedora Related to Fedora ecosystem complexity/single-task Regular task, should be done within days. gain/high This brings a lot of value to (not strictly a lot of) users. impact/low This issue impacts only a few users. kind/feature New feature or a request for enhancement.
Projects
Archived in project
Development

No branches or pull requests

4 participants