GitLab events / parsing refactoring #2590

mfocko · 2024-10-21T12:44:16Z

TODO

Write new tests or update the old ones to cover new functionality.
Update doc-strings where appropriate.
Open up follow-up issues.

We create instances of abstract classes in tests… I have implemented .event_type() for those with an assertion to check that the tests are actually running…

Comments / questions from the commits

Manual caching of the db_project_object, db_project_event and project (currently unimplemented method in superclasses, property that caches to an attribute)
Comment from the review:
```
Are these supposed to be abstract methods?
Also, if they're abstract (and cached, based on the commits from blame),
why don't we just use cachedproperty after abstractmethod? Having these
return ‹None› by default »can« result in unimplemented override somewhere
down the chain.
```
- Leaving as is for now, both project_object and project_event are constructed at the same time via one helper function, therefore the caching can't be simply wrapped with a cached_property
- Ideally drop explicit getters and implement cached abstract properties (even though I recall some issues with type checking that, or it's not very bulletproof…)
Event.pre_check returns True by default…
- after a discussion with @lbarcziova just adding a note why we have chose the specific default there
- log a warning
- default to False

Reconsider structuring

forge.push.Push (respectively gitlab.push.TagPush) → forge.push.Commit or forge.push.Tag
anitya.base → anitya.events (not used directly though, events are exposed via __init__.py as it doesn't make much sense to structure them further)
forge.pr.Synchronize doesn't make sense, it is “some kind of an action” on the PR/MR, but Synchronize has been inspired by the GitHub itself (and it's an example of an action on the PR), but I don't have a better name… maybe even forge.pr.Change / forge.pr.Action?
Enumerations: there are some enums in events.enums for PR/MRs, comments and FedMsg…
- Combination of both… as for the PR and comment actions they are shared by GitHub and Pagure, therefore I kept those shared actions in the events.enums, while moving the GitLab enum that's also shared by GitLab-only events to events.gitlab.enums and as for the OpenScanHub, the least disruptive change (since the enum is tied only to the task) to events.openscanhub.task.Status
- Follow OpenScanHub implementation by Maja: nests the enumerations into the event type itself, which is a possible way, but it looks a bit weird used in code
- Have a separate module (e.g. openscanhub.enum) for all of them consistently
- Going from the current change:
```
-        if self.event.status == OpenScanHubTaskFinishedEvent.Status.success:
+        if self.event.status == openscanhub.task.Finished.Status.success:
```
  I'd probably prefer openscanhub.task.Status.success 👀

Fixes

Supersedes #2586 (cause GitHub is a 🫓)

Merge before/after

softwarefactory-project-zuul · 2024-10-21T12:48:31Z

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

✔️ pre-commit SUCCESS in 2m 16s
❌ packit-service-tests FAILURE in 2m 36s

mfocko · 2024-10-21T12:48:46Z

@majamassarini @lbarcziova WDYT? At the moment it's broken everywhere possible :D it looks like we have also some pretty intrusive mocking in place, so this reshuffle opened the can of worms…

I still haven't made up my mind about the usage of the events, cause they can be imported as

from packit_service.worker.events.github.pr import Synchronize

but at the same time I have a feeling that it won't look very good once it gets mixed up with the other forges, for now I replaced the imports with as ‹original_name›, so that it breaks as least stuff as possible, but going forward it doesn't feel very feasible… it probably needs a better hierarchy 🤔

mfocko · 2024-10-21T13:19:36Z

Tip

I had a feeling something will crawl out, when I make Event an abstract class

packit_service/worker/events/github/installation.py:41: error: Cannot instantiate abstract class "Installation" with abstract attributes "base_project" and "get_packages_config"  [abstract]

softwarefactory-project-zuul · 2024-10-21T13:40:42Z

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

✔️ pre-commit SUCCESS in 2m 19s
❌ packit-service-tests FAILURE in 2m 30s

softwarefactory-project-zuul · 2024-10-21T13:51:12Z

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

✔️ pre-commit SUCCESS in 2m 15s
❌ packit-service-tests FAILURE in 2m 36s

lbarcziova

the structure looks good to me 🚀

for now I replaced the imports with as ‹original_name›, so that it breaks as least stuff as possible, but going forward it doesn't feel very feasible

agreed it may be a good middle step

softwarefactory-project-zuul · 2024-10-25T12:45:54Z

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

✔️ pre-commit SUCCESS in 2m 12s
❌ packit-service-tests FAILURE in 2m 39s

majamassarini · 2024-10-28T08:19:07Z

@majamassarini @lbarcziova WDYT? At the moment it's broken everywhere possible :D it looks like we have also some pretty intrusive mocking in place, so this reshuffle opened the can of worms…

I still haven't made up my mind about the usage of the events, cause they can be imported as
from packit_service.worker.events.github.pr import Synchronize
but at the same time I have a feeling that it won't look very good once it gets mixed up with the other forges, for now I replaced the imports with as ‹original_name›, so that it breaks as least stuff as possible, but going forward it doesn't feel very feasible… it probably needs a better hierarchy 🤔

Thanks for starting this big refactoring, to me it looks good!
If I understand your concern, I think in future we should avoid import like this

from packit_service.worker.events.github.pr import Synchronize

and use instead the namespaces we have created

from packit_service.worker.events import github, gitlab, pagure
[...]
github.pr.Synchronize
[...]
gitlab.pr.Synchronize

So I think the hierarchy is fine 🤔

mfocko · 2024-10-28T08:23:57Z

Thanks for starting this big refactoring, to me it looks good! If I understand your concern, I think in future we should avoid import like this
from packit_service.worker.events.github.pr import Synchronize
and use instead the namespaces we have created
from packit_service.worker.events import github, gitlab, pagure
[...]
github.pr.Synchronize
[...]
gitlab.pr.Synchronize
So I think the hierarchy is fine 🤔

oh i see, so you really wanna use it like that 😁 I was a bit worried how explicit it will be, but I guess it's better

edit: BTW I managed to break the mypy locally, it crashes :D but passes in Zuul…

softwarefactory-project-zuul · 2024-10-28T10:42:29Z

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

✔️ pre-commit SUCCESS in 2m 05s
❌ packit-service-tests FAILURE in 7m 11s

softwarefactory-project-zuul · 2024-11-25T12:55:14Z

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

✔️ pre-commit SUCCESS in 2m 03s
❌ packit-service-tests FAILURE in 2m 55s

softwarefactory-project-zuul · 2024-11-25T13:09:21Z

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

✔️ pre-commit SUCCESS in 2m 03s
❌ packit-service-tests FAILURE in 2m 42s

softwarefactory-project-zuul · 2024-11-26T15:56:55Z

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

✔️ pre-commit SUCCESS in 2m 10s
❌ packit-service-tests FAILURE in 2m 45s

softwarefactory-project-zuul · 2024-12-03T14:11:09Z

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

✔️ pre-commit SUCCESS in 2m 08s
❌ packit-service-tests FAILURE in 2m 51s

softwarefactory-project-zuul · 2025-01-06T11:34:51Z

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

✔️ pre-commit SUCCESS in 1m 47s
❌ packit-service-tests FAILURE in 1m 52s

lbarcziova

I really like how you broke this up into smaller pieces, this is so much more readable, good job!

packit_service/worker/events/event.py

packit_service/worker/events/github/installation.py

packit_service/worker/events/koji.py

softwarefactory-project-zuul · 2025-01-13T14:42:54Z

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

✔️ pre-commit SUCCESS in 1m 45s
❌ packit-service-tests FAILURE in 1m 48s

softwarefactory-project-zuul · 2025-01-15T12:47:27Z

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

✔️ pre-commit SUCCESS in 1m 50s
❌ packit-service-tests FAILURE in 2m 03s