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

Skip actor pre_check if actor not present #1733

Merged

Conversation

lbarcziova
Copy link
Member

This should make the behaviour the same as it was previously (before handler pre_checks refactoring

if self.event.actor and not handler.check_if_actor_can_run_job_and_report(
): if the actor is None, the actor pre_check will be skipped (e.g. pushes to branch, releases).


RELEASE NOTES BEGIN

RELEASE NOTES END

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 1m 56s
✔️ packit-service-tests SUCCESS in 2m 01s
✔️ packit-service-tests-openshift SUCCESS in 11m 55s

@mfocko
Copy link
Member

mfocko commented Nov 1, 2022

But someone »does« the push or release, don't they?

@lbarcziova
Copy link
Member Author

If I recall correctly, we previously did the check only for PRs since if a person commits or releases, he/she has also write access to the repo.

Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 40 to 43
if not self.actor:
return False
logger.debug("Actor not set for this event, skipping the actor check.")
return True
return self._pre_check()
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we checking also other non-actor conditions? (I went through the code really quickly and am not sure about the pre_check / _pre_check call chain...)

Copy link
Member

Choose a reason for hiding this comment

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

e.g. project allowed for internal TF,..

Copy link
Member Author

Choose a reason for hiding this comment

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

I think only _pre_checks in subclasses here or here are called, but I would wait for Maja's approval since she knows the code the best

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Laura for fixing this.
I did not realize I had changed the actor checker behaviour. You are right, before my changes it would have returned False only with a valid actor field.

Before my changes this code were called for every handler, but most of them just returned True, now it is called just for this two handlers Laura linked here.

This should make the behaviour the same as it was previously
(before handler pre_checks refactoring
https://github.com/packit/packit-service/blob/da94cc9b511843d73e9b94d20677f0b1d1426bf8/packit_service/worker/jobs.py#L422):
if the actor is None, the actor pre_check will be skipped (e.g.
pushes to branch, releases).
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ pre-commit SUCCESS in 2m 05s
✔️ packit-service-tests SUCCESS in 2m 05s
✔️ packit-service-tests-openshift SUCCESS in 12m 42s

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

Build succeeded (gate pipeline).

✔️ pre-commit SUCCESS in 1m 59s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit b31d6a9 into packit:main Nov 2, 2022
@lbarcziova lbarcziova deleted the actor-check-fix branch November 2, 2022 08:13
nforro pushed a commit that referenced this pull request Nov 2, 2022
Skip actor pre_check if actor not present

This should make the behaviour the same as it was previously (before handler pre_checks refactoring

      packit-service/packit_service/worker/jobs.py

         Line 422
      in
      da94cc9

           if self.event.actor and not handler.check_if_actor_can_run_job_and_report(

): if the actor is None, the actor pre_check will be skipped (e.g. pushes to branch, releases).

RELEASE NOTES BEGIN
RELEASE NOTES END

Reviewed-by: Matej Focko <None>
Reviewed-by: None <None>
Reviewed-by: Laura Barcziová <None>
Reviewed-by: Maja Massarini <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit When set, zuul wil gate and merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants