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.

Sign up for GitHub

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

Jump to bottom

Feat(bodhi): Retrigger bodhi update via dist-git PR command #1729

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Feat(bodhi): Retrigger bodhi update via dist-git PR command #1729

merged 1 commit into from
Nov 10, 2022

Conversation

majamassarini
Copy link
Member

@majamassarini majamassarini commented Oct 28, 2022 •
edited
Loading

I have rebased the Jirka K. work and I have used Mixins to manage Koji data needed to create a new bodhi update.

This comes from #1654

Fixes #1541

Merge after packit/packit#1764

Note: since I am using this method if for any reason the actor is None then the body update will succeed, I don't know if this could be a problem.

RELEASE NOTES BEGIN
Packit now allows to re-trigger Bodhi update via dist-git PR comment /packit create-update.
RELEASE NOTES END

@majamassarini majamassarini marked this pull request as draft October 28, 2022 13:15
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 00s
✔️ packit-service-tests SUCCESS in 6m 54s
✔️ packit-service-tests-openshift SUCCESS in 16m 13s

@majamassarini majamassarini force-pushed the retrigger-bodhi branch 2 times, most recently from f98d1d5 to b365a66 Compare November 3, 2022 09:56
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ pre-commit SUCCESS in 2m 01s
❌ packit-service-tests FAILURE in 1m 58s
✔️ packit-service-tests-openshift SUCCESS in 13m 00s

@majamassarini
Copy link
Member Author

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 00s
✔️ packit-service-tests SUCCESS in 2m 00s
✔️ packit-service-tests-openshift SUCCESS in 13m 33s

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 02s
✔️ packit-service-tests SUCCESS in 1m 58s
✔️ packit-service-tests-openshift SUCCESS in 12m 50s

@majamassarini majamassarini mentioned this pull request Nov 4, 2022
Merged
@majamassarini majamassarini marked this pull request as ready for review November 4, 2022 11:01
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 05s
✔️ packit-service-tests SUCCESS in 2m 03s
✔️ packit-service-tests-openshift SUCCESS in 11m 08s

TomasTomecek
TomasTomecek reviewed Nov 8, 2022
View reviewed changes
Copy link
Member

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the Checkers design! This is nicely done, just one question about the UX

packit_service/worker/handlers/bodhi.py
Comment on lines +216 to +221
logger.debug("Bodhi update will be re-triggered via dist-git PR comment.")
return (
HasAuthorWriteAccess,
IsAuthorAPackager,
IsKojiBuildCompleteAndBranchConfiguredCheckService,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will Packit provide any feedback back to the user if one of these checkers fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, there is no feedback for the users when checks fail. For none of the them.
If you agree I will create a new card to add the feedback, I think there is some work on it. Right now there is no BodhiUpdateJobHelper class and for the other services is this kind of helper class that handles the feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed! we should absolutely have something to comment back so users know what's happening in Packit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed! we should absolutely have something to comment back so users know what's happening in Packit

#1747

lbarcziova
lbarcziova reviewed Nov 8, 2022
View reviewed changes
Copy link
Member

@lbarcziova lbarcziova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done!

packit_service/worker/handlers/mixin.py Show resolved Hide resolved
@lbarcziova
Copy link
Member

Note: since I am using this method if for any reason the actor is None then the body update will succeed, I don't know if this could be a problem.

We should definitely make sure the actor is set, but looking into the code here and here (we could finally unify so that all events have the actor and not user_login, this is not very straightforward :D), I think this should be ok.

@majamassarini
Copy link
Member Author

Note: since I am using this method if for any reason the actor is None then the body update will succeed, I don't know if this could be a problem.

We should definitely make sure the actor is set, but looking into the code here and here (we could finally unify so that all events have the actor and not user_login, this is not very straightforward :D), I think this should be ok.

I am not sure I am following here 😅
The code you linked is not straightforward but it should work, as you said.
But I am worried about an event having a None actor. Like those event that made you fix this: #1733.
It was ok to return True for those checks, now I am wondering if I can behave in the same way here.
Or maybe I have misunderstood the reason for the fix?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 1m 52s
✔️ packit-service-tests SUCCESS in 1m 53s
✔️ packit-service-tests-openshift SUCCESS in 11m 34s

@lbarcziova
Copy link
Member

But I am worried about an event having a None actor.

I understand and the code I linked was meant to show it should be always not-none, or am I missing something? 😅 (My fix only made the behaviour the same as it was in the past)

lbarcziova
lbarcziova approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@lbarcziova lbarcziova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@majamassarini majamassarini mentioned this pull request Nov 9, 2022
Closed
softwarefactory-project-zuul bot added a commit to packit/packit that referenced this pull request Nov 10, 2022
@softwarefactory-project-zuul
Merge pull request #1764 from majamassarini/fix/packit-service/1654
5c0aea9 
Add methods to get build dicts from KojiHelper

Merge before packit/packit-service#1729

Reviewed-by: Laura Barcziová <None>
Reviewed-by: Tomas Tomecek <[email protected]>
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 05s
✔️ packit-service-tests SUCCESS in 2m 02s
✔️ packit-service-tests-openshift SUCCESS in 11m 03s

@majamassarini majamassarini added the mergeit When set, zuul wil gate and merge the PR. label Nov 10, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ pre-commit SUCCESS in 2m 00s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit a53ff50 into packit:main Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomasTomecek TomasTomecek TomasTomecek left review comments

@lbarcziova lbarcziova lbarcziova approved these changes

@nikromen nikromen Awaiting requested review from nikromen

Assignees
No one assigned
Labels
mergeit When set, zuul wil gate and merge the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement retriggering Bodhi updates via dist-git pull-request comments
4 participants
@majamassarini @lbarcziova @TomasTomecek @nikromen