-
-
Notifications
You must be signed in to change notification settings - Fork 2
[18.0][ADD] vcp #3
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
base: 18.0
Are you sure you want to change the base?
Conversation
e9c3515 to
18d1adc
Compare
|
Oh yeah! I am going to review and to contribute on it. Thanks ! |
18d1adc to
a3bf522
Compare
vcp_github/models/res_partner.py
Outdated
| github_organization = fields.Boolean( | ||
| string="Is GitHub Organization", | ||
| help="Check if this partner represents a GitHub organization", | ||
| readonly=True, | ||
| ) | ||
| github_user = fields.Boolean( | ||
| string="Is GitHub User", | ||
| help="Check if this partner represents a GitHub user", | ||
| readonly=True, | ||
| ) |
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.
Proposal :
| github_organization = fields.Boolean( | |
| string="Is GitHub Organization", | |
| help="Check if this partner represents a GitHub organization", | |
| readonly=True, | |
| ) | |
| github_user = fields.Boolean( | |
| string="Is GitHub User", | |
| help="Check if this partner represents a GitHub user", | |
| readonly=True, | |
| ) | |
| github_user_type = fields.Selection( | |
| selection=[("user", "User"), ("organization", "Organization")], | |
| readonly=True, | |
| ) |
If I understand correctly, a github element can be an org or a user, not both.
don't you think ?
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.
Yes, but I did it this way because we use orgs as origin of the PR, and it can come from a user or from an org (some users are marked as organizations). However, I think your solution is cleaner (we might need to improve the view and so on)
…s sense to have a plateform without kind + it makes failing call of update_information AttributeError: 'vcp.platform' object has no attribute '_update_information_False'. Did you mean: '_update_information_github'?
Co-authored-by: Sylvain LE GAL <sylvain.legal@grap.coop>
legalsylvain
left a comment
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.
Some remarks inline.
Otherwise, LGTM. Thanks !
| def _cron_update_platforms(self): | ||
| for organization in self.search([]): | ||
| try: | ||
| organization.update_information() | ||
| except Exception as e: | ||
| _logger.error( | ||
| "Error updating organization %s: %s", organization.name, str(e) | ||
| ) |
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.
| def _cron_update_platforms(self): | |
| for organization in self.search([]): | |
| try: | |
| organization.update_information() | |
| except Exception as e: | |
| _logger.error( | |
| "Error updating organization %s: %s", organization.name, str(e) | |
| ) | |
| def _cron_update_platforms(self): | |
| for platform in self.search([]): | |
| try: | |
| platform.update_information() | |
| except Exception as e: | |
| _logger.error( | |
| "Error updating platform %s: %s", platform.name, str(e) | |
| ) |
|
|
||
| @api.depends() | ||
| def _compute_vcp_contributions(self): | ||
| self.filtered(lambda p: p.github_user)._compute_vcp_contributions_field( |
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.
hum. I think there is a little mistake in the design here. You call github_user field, but this one is defined in vcp_github module.
(same in line 33)
vcp_github/models/res_partner.py
Outdated
|
|
||
| _sql_constraints = [ | ||
| ( | ||
| "github_name_uniq", |
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 a problem with that constraints, when I tested this module.
(The constraints, makes sense)
Use case :
- I have a database, with a partner named "sylvain LE GAL" (id#X)
- I install this module, and launch the fetch of github.
- As I can not set github_name on partner (github_name is readonly), the cron creates a new partner, named 'legalsylvain'. id#Y)
- When I try to merge the two partners. I have an error.
What do you think. This will be a common use case, IMO.
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.
@legalsylvain this comment will interest you #3 (comment)
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.
Yes. I had the same problem with the previous github_connector implementation. I implemented also a function to create partner on the fly. But there was some duplicates. I'm not sure what is the best solution.
sebastienbeau
left a comment
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.
Thanks for you work, tell me what do you think about adding more granularity.
Note: on my side I was using Pygithub. I like doc and the way to get pull_request fell more python https://pygithub.readthedocs.io/en/stable/examples/PullRequest.html#get-pull-requests-by-query but It's a matter of taste, so I will follow you on whatever you choose
| _name = "vcp.platform" | ||
| _description = "VCP Platform" |
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.
Maybe a better name will be "vcp.orga" or "vcp.organization" ?
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.
well, actually, after some review I think we should do
vcp.platform.kind(Github, Gitlab, my own gitlab...)vcp.platform: The organization that we are analyzingvcp.organization: For handling companies
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 was asking myself the same question this night about vcp.platform and vcp.organization, in both case it's an organization but we do not have same usage. On github is the same table, but in Odoo we do not want to pollute the table of "vcp.platform" that we want to download all repo. I was thinking about having one table with a flag and a domain to hide "organization" in the menu, but it maybe a little breakable. So let's go to have "vcp.platform" and "vcp.organization"
vcp_github/models/res_partner.py
Outdated
| partner.github_organization = True | ||
| return partner.id | ||
|
|
||
| def _get_github_user(self, gh, client): |
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 am not sure that it's a good idea to create on the fly the partner.
At the initial import, some contributor may exist in the database but without having an existing github_login fill. So all PR will have the "duplicated partner" and we will need to merge it.
Also In the RFQ2 of the datamodel (see here: https://drive.google.com/file/d/1YCIce01J7P9unxAQ51XT7BqBIuZrOr9m/view) it's was required that a member can have several "github user". I am not sure that this case really exist in the reality, but it's was required.
On my side I was thinking about adding a simple model "vcp.user" (with some basic field and a link to a "partner_id") that will be created on the fly but then if it's linked to a "res.partner" we can compute the "partner_id" of the PR
The advantage I see:
- it's does create useless partner (In case of feature of partner-module-information, it do not want to have all oca contributor as partner in my ERP)
- it's allow to map the partner after
- it's allow to have several github-user
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.
We may use the same concept for the organization
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.
👍
| for pr in clients[i].search_issues( | ||
| f"is:pr repo:{self.platform_id.name}/{self.name} " | ||
| f"updated:{start.isoformat()}..{end.isoformat()}" | ||
| ): | ||
| i = (1 + i) % len(clients) |
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.
supporting multi key for solving rate limit ;) 👍
| def _parse_github_pr(self, pr, client): | ||
| origin_data = pr.as_dict() | ||
| comments_url = pr.comments_url | ||
| comments_req = client.session.get(comments_url) | ||
| comments = comments_req.json() | ||
| while comments_req.links.get("next"): | ||
| comments_url = comments_req.links["next"]["url"] | ||
| comments_req = client.session.get(comments_url) | ||
| comments += comments_req.json() | ||
| reviews_url = pr.reviews().url | ||
| reviews_req = client.session.get(reviews_url) | ||
| reviews = reviews_req.json() |
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.
As my aim is to reuse this module in other context
- gitlab interaction with project management
- module-partner-information : (for organizing the PR management + timesheet)
- odoo-repository to remove duplicated code
I was think about splitting more the module (so you can only depend on what you need)
To keep in mind
-
odoo-repository: only need the logic of vcp.plateform (or vcp.organisation), vcp.repository, vcp.branch
-
module-partner-information: same as odoo-repository + PR (without comment)
-
oca stats: need all
So my initial idea was to split a little more the module
- vcp
- vcp_pr
- vcp_pr_activity ( comment + review)
and so
- vcp_github
- vcp_github_pr
- vcp_github_pr_activity
To avoid a nightmare for the portal, I think we can include all the portal logic in vcp_pr_activity or creating a module vcp_portal that depend on vcp_pr_activity. But the idea is that the portal do not make sense without the activity information so we do need to split everything for the portal
What do you think ?
Note: I am not asking to do it (you already have done a lot), I can do it. I just want to get your point of view to go it the right direction
Note 2: I like the idea of having a portal for the member, and as we will have all the PR, I think we can improve it so we can show all the pending PR that you need to review, or all PR where somebody as you to fix it (I was talking about his with @legalsylvain some month ago)
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 will try to do the split, however, IMO, it has no sense to split pr from reviews and comments, they are quite aligned.
If as a company I want to monitor the PRs of OCA, it makes sense to know who approved them 😉
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.
Maybe @legalsylvain is right and we could do that by configuration (it might be simpler)
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 should be good to state clearly the goals /no goal of each project.
There is a high risk to build a gaz factory: hard to maintain, impossible to refactor.
what are odoo-repository and module-partner-information ? could you provide links ? |
Wahou that are a lot of split !
What do you think ? |
Here are the links : |
Having an option is the minimal needed (so we do not download all comment + review if you do not need it). I would like to have the feedback of @sebalix as the idea is to depend on this for the odoo-repository project (to avoid re-implementing the logic of orga, repo) I tend to prefer smaller module so it allow to share the migration effort. When you have a simple module as dependency you migrate it (less code, less work). If it does a lot of thing, you may wait for other to do it. But I will not block anything, I just share my point of view. |
|
The problem with your comment is that implies a lot of extra glue modules, we might need to find the best option that makes it feasible and not too complicated. For example, if we split too much, we might need vcp vcp_pr, vcp_review, vcp_github, vcp_github_pr, vcp_github_review.... that is the reason I commented, we need to find something that doesn't requires so many modules. |
Thinking twice, I tend to agree with you, splitting to much will add to much glue module, just having an option (on the vcp.plateform) can be enough for my case. And extracting the vcp_portal like you did already simplify the base module so it's not a big deal to migrate the comment and review. |
|
|
||
| class VcpBranch(models.Model): | ||
| _name = "vcp.branch" | ||
| _description = "Branch" |
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.
What is a branch exactly? Are we talking about a real git branch name, or an Odoo release/serie/version?
Asking as I faced this name issue in https://github.com/OCA/module-composition-analysis/blob/16.0/odoo_repository/models/odoo_branch.py which deserves a renaming to odoo.serie or odoo.version, but now the goal is to re-use your modules. In that project a serie/version is 17.0/18.0/19.0, while a branch can be main/18.0-mig which is linked to a serie/version.
@sebastienbeau you were proposing the term Odoo "serie" IIRC
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 Branch is a Branch in git. We should need to keep it, because there are repositories that are not using odoo versions (master/main for example on libraries like openupgradelib)
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.
@sebalix I think we should split the concept of "odoo.serie" and "vcp.branch".
An "odoo.serie" is the major version of odoo (17.0, 18.0, 19.0...)
A "vcp.branch" is a branch in term of git.
For sure in OCA case the branch (for repo that are odoo modules) are linked to one "odoo.version" that have the same name (but maybe one days if odoo start to be stable, we may have branch like 20+ that work with several version)
So my idea is that in odoo-repository we name this "odoo.version" and "odoo.serie" and then a branch is link to a serie (18.0-mig have the odoo.serie 18.0)
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.
All clear, wanted to know what it was about 👍
@sebastienbeau I think you could enjoy this one 😄