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
CentOS integration initial work #515
Conversation
This pull request introduces 2 alerts when merging 0034509 into 84e73ca - view on LGTM.com new alerts:
|
Build failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice progress!
This pull request introduces 2 alerts when merging 98b49bb into 5035fcc - view on LGTM.com new alerts:
|
Build failed.
|
Build failed.
|
Build failed.
|
This pull request introduces 2 alerts when merging 013818f into c4b2d56 - view on LGTM.com new alerts:
|
Build failed.
|
This pull request introduces 1 alert when merging 7ad51cb into be2b592 - view on LGTM.com new alerts:
|
Build failed.
|
This pull request introduces 1 alert when merging 75d0618 into c2a7bcf - view on LGTM.com new alerts:
|
Build succeeded.
|
This pull request introduces 1 alert when merging 830745d into c2a7bcf - view on LGTM.com new alerts:
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see tests with real event json payloads
This pull request introduces 1 alert when merging bdea325 into c2a7bcf - view on LGTM.com new alerts:
|
Build failed.
|
|
||
@use_for(job_type=JobType.copr_build) | ||
@use_for(job_type=JobType.build) | ||
class PagurePullRequestCoprBuildHandler(AbstractPagureCoprBuildHandler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this class intended to be responsible for starting builds when a pull-request is filed/updated in CentOS infra? If yes, could we call it simply PullRequestHandler
? This name is too long, and it has too many details that can be inferred from the context or is irrelevant at this level of abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have followed current architecture.
There are different implementation how we are getting configs for GitHub and Pagure because of different api capabilities.
Event.get_package_config() and AbstractGithubJobHandler/AbstractPagureJobHandler (via GithubPackageConfigGetter/PagurePackageConfigGetteris) GitService type dependent, thus preventing of using more general class. I think this breaking of SRP, i will raise arch day topic for this.
return result | ||
|
||
|
||
class PullRequestPagureEvent(AddPullRequestDbTrigger, AbstractPagureEvent): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A diff with PullRequestEvent
shows very little difference. Why is the reason behind this code duplication?
This is also a question for the other *Event
classes added.
@@ -1,4 +1,4 @@
-class PullRequestEvent(AddPullRequestDbTrigger, AbstractGithubEvent):
+class PullRequestPagureEvent(AddPullRequestDbTrigger, AbstractPagureEvent):
def __init__(
self,
action: PullRequestAction,
@@ -9,7 +9,7 @@ class PullRequestEvent(AddPullRequestDbTrigger, AbstractGithubEvent):
target_repo: str,
https_url: str,
commit_sha: str,
- github_login: str,
+ user_login: str,
):
super().__init__(trigger=TheJobTriggerType.pull_request, project_url=https_url)
self.action = action
@@ -19,7 +19,7 @@ class PullRequestEvent(AddPullRequestDbTrigger, AbstractGithubEvent):
self.base_ref = base_ref
self.target_repo = target_repo
self.commit_sha = commit_sha
- self.github_login = github_login
+ self.user_login = user_login
self.identifier = str(pr_id)
self.git_ref = None # pr_id will be used for checkout
@@ -31,8 +31,7 @@ class PullRequestEvent(AddPullRequestDbTrigger, AbstractGithubEvent):
def get_package_config(self) -> Optional[PackageConfig]:
package_config: PackageConfig = self.get_package_config_from_repo(
project=self.get_project(),
- reference=self.base_ref,
- pr_id=self.pr_id,
+ reference=self.commit_sha,
fail_when_missing=False,
)
if not package_config:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those differences are crucial and it is most natural and also clean way to implement it in with current design.
I agree that there is unwanted duplication, but there will be required redesign to avoid it and it is out of scope of this task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This discussion is similar to my previous comment -- if it is easy to do it here, I would merge them. If it requires big changes in the other code, let's leave it as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reply to previous comment applies also here:
There is different ConfigGetter class implementation and this should be object of refactoring - this shouldn't be part of Event (its breaking SRP and creating tight coupling). This needs discussion and also i think it is out of scope of this PR.
This mentioned above this should be discussed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you now have user_login
also in GitHub events, what about moving the common code to the new superclass?
These classes can inherit from the superclass and config getter class. We can definitely refactor later, but this deduplication should be really easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, leaving approve to Hunor since he raised some interesting points
This pull request introduces 1 alert when merging a17cf85 into 0b318a6 - view on LGTM.com new alerts:
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies @sakalosj but I cannot approve this to be merged. The reason is the following:
So far code in packit-service was written to serve one git-forge, and that git-forge was GitHub. This made the architecture opinionated in many ways, making integrating with another git-forge difficult.
But that's OK, and something that we should expect to happen in a world where requirements change.
My expectation in a situation like this is to see some architecture work and refactoring done first, in order to enable integrating with the new git-forge in a more natural way.
I cannot see this work happening in this PR.
The reason skipping this work now and merging this is as is problematic b/c it postpones important and still undefined work to the unknown future.
At this moment we don't know what we would need to fix, how much effort that fix would be and when that fix can be completed. What we do now, is that we will need to deliver new features and functionality which should build on top of this work, and which, if needs to be completed before the refactor can happen, will further contribute to the increase of this technical debt.
So merging this IMHO equals with pushing a huge technical debt in our backlog. (Note, that this PR has 22 files changed, 539 insertions and 91 deletions, so it's not something trivial in size either.) And I just cannot see the value for doing such a compromise at this point.
I also see blaming the current architecture for a bad solution a very weak argument.
If others from the team do approve, I'll accept that decision. But no green tick mark from me this time, sorry.
On the other hand, I would gladly help to come up with a better solution than the current one.
|
@csomh thanks for the thorough and honest write-up I agree this PR adds technical debt to the codebase. The question here is:
Nevertheless, someone needs to think and come up with a solution to refactor the codebase so things are more clear - who's gonna do that? when and how? My main worry is that if we keep polishing this PR, it will take weeks to merge and we'll get "behind the schedule" also adding frustration to Jano since he'd need to rebase and keep on iterating. Doing subsequent PRs with smaller changes should be easier to review and we should see the benefits immediately, rather than reviewing Hence, my preference would be to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to support @sakalosj with his approach.
We already discuss the topic if we want to do it in the old way or try to improve how the service works. That was me who wanted to wait with the refactoring to arch-day.
Refactor first | Use current approach and refactor later |
---|---|
➕ CentOS support can be added in a cleaner way | ➖ Current way does not look good, but ➕ we will see what is common and what differs when refactoring. |
➖ only Jano will work on it (?), it will be done only partially | ➕ We can properly restructure the service and discuss that on the arch-day => what was our plan already |
➖ The refactor needs to be done (and approved) before this => will not make it before Summit or don't make it well. | ➕ We will make this to Summit. 🎉 |
I am not scared of these changes -- there are mostly additions and deduplication can be easily done later.
edit: If we want to merge these features in 100% shape, we will never merge them.
return ServiceConfig.get_service_config().get_project(url=self.project_url) | ||
|
||
|
||
class PushPagureEvent(AbstractPagureEvent): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those event classes look very similar to Github ones:
- It would be nice to merge them.
- We can make a subclass just for the differentiation without any code change if we really want to differentiate that.
- Since we have the
https_url
, we can still be able to get service easily.
- Since we have the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is different ConfigGetter class implementation and this should be object of refactoring - this shouldn't be part of Event (its breaking SRP and creating tight coupling). This needs discussion and also i think it is out of scope of this PR.
return result | ||
|
||
|
||
class PullRequestPagureEvent(AddPullRequestDbTrigger, AbstractPagureEvent): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This discussion is similar to my previous comment -- if it is easy to do it here, I would merge them. If it requires big changes in the other code, let's leave it as is for now.
packit_service/worker/jobs.py
Outdated
@@ -179,8 +181,13 @@ def process_jobs(self, event: Event) -> Dict[str, HandlerResults]: | |||
# failed because of missing whitelist approval | |||
whitelist = Whitelist() | |||
github_login = getattr(event, "github_login", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename the github_login
to user_login
as well to have the same API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
This pull request introduces 1 alert when merging ebe4741 into ca2b124 - view on LGTM.com new alerts:
|
Build succeeded.
|
No description provided.