Draft: GitLab events / parsing refactoring #2586
Conversation
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 30s |
There was a problem hiding this comment.
Thank you 🙏🏻
Looking at this I think I would like to have more namespaces in our code. I would like to have github.PullRequestEvent or even better event.github.PullRequest instead of GithubPullRequestEvent. I don't know what others think about it and I don't know if this could be the right moment for such a big refactoring... but still I think it would be nicer 🙂
majamassarini
dismissed
their stale review
Sorry I didn't realize it was still a draft.
I'm thinking about it too :D and I will need to do some changes around anyways, so… the biggest issue is fixing the imports afterwards :D |
I think it will break everything, so… let's do it \o/ |
|
How about… @lbarcziova @majamassarini @packit/the-packit-team The bigger issue how to fit the downstream events in there… Do they even count as downstream-only if they come from both GitLab and Pagure? It almost feels like there should be two parsers (parsing Fedora Messaging event) returning the same type (downstream event) regardless of whether it's from GitLab or Pagure. We shouldn't even care about this… I don't want to add another layer of |
you are right, this is probably how it should be done the most correctly but otherwise the proposal looks good to me! Is it manageable to have it like that considering your note about upstream vs downstream? @mfocko |
maybe ? I think that for the beginning I could just rename Pagure to downstream (hide the fact it's pagure) and try to make it more abstract for supporting GitLab (or potentially Forgejo too, if and when Fedora migrates) |
mfocko
force-pushed
the
fix/gitlab-dg-events
branch
from
a472ed8 to
89c111a
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 18s |
|
GitHub PLEASE /o |
GitLab events / parsing refactoring 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 Reviewed-by: Laura Barcziová Reviewed-by: Maja Massarini Reviewed-by: Matej Focko
TODO:
packit/packit.dev.Fixes #2471
Related to
Merge before/after
RELEASE NOTES BEGIN
Packit now supports automatic ordering of ☕ after all checks pass.
RELEASE NOTES END