Skip to content

fix(gitlab): remove double call for MR assignees #14212

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

Merged
merged 13 commits into from
Feb 16, 2022

Conversation

ExNG
Copy link
Contributor

@ExNG ExNG commented Feb 14, 2022

Changes:

Gitlab Enterprise On-Prem allows multiple assignees for MRs, see https://docs.gitlab.com/ee/api/merge_requests.html#update-mr .
This PR removes the call for a single assignment when the multiple assignees are set, with a fallback for free Gitlab users, thus greatly reducing notifications from GL.

Context:

If more than one assignee is set for renovate it'll first call the API with the first assignee and then all assignees again in one separate call, thus gitlab sends out a lot of emails and notifies users twice.

Screenshot from 2022-02-14 12-37-23

We really appreciate renovate and hope this change will improve the UX for other Gitlab enterprise users.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@ExNG ExNG changed the title Fix double call for multiple Gitlab MR assignees fix: double call for multiple Gitlab MR assignees Feb 14, 2022
@rarkins rarkins changed the title fix: double call for multiple Gitlab MR assignees fix(gitlab): remove double call for MR assignees Feb 14, 2022
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I don't like the implementation. We need to use the current implementation if we don't know if we can use the premium api.

If we can detect the premium api, we should do that. Otherwise this needs to be configurable via a platform/ experimental option.

@ExNG
Copy link
Contributor Author

ExNG commented Feb 14, 2022

Thanks for the quick response!

I think the best way to archive this would be a new method in lib/platform/gitlab/index.ts called something like async isEE(): Promise<boolean>.
This method should act as a singleton and call /version (see https://docs.gitlab.com/ee/api/version.html), in there is the gitlab version and its edition as suffix.
There seems to be -ee for enterprise edition and -pre, i need to do some research before i can tell what the community edition has and what suffix includes the assignees_ids key for merge requests.

Please tell me what you think of this approach :)

@viceice
Copy link
Member

viceice commented Feb 14, 2022

That will probably be not enough, as the gitlab version doesn't know which license seat the current renovate user has.

The assignees feature is a licensing thing.

@ExNG
Copy link
Contributor Author

ExNG commented Feb 14, 2022

Okay thanks, i will look what i can do on my end and inform you tomorrow.
Schoenen Feierabend! ^^

@rarkins
Copy link
Collaborator

rarkins commented Feb 14, 2022

What about on gitlab.com - isn't it mixed and based in plan?

@ExNG
Copy link
Contributor Author

ExNG commented Feb 15, 2022

Gitlab SaaS and self-hosted are more or less the same except for a handful of features.

Coming back to the Paid plans problem, which indeed only applies to the user, not the instance.
I think there currently is real way of telling if the user is (if hes not admin, which shouldn't be a requirement for bot) in any paid plan except for testing API of paid features. More on this here: https://docs.gitlab.com/ee/api/users.html#for-admin

I will go ahead and add a config flag for Gitlab Plans which accepts free, premium and ultimate, might also be useful for others as some Gitlab features are Ultimate only.

@Brcrwilliams
Copy link

Brcrwilliams commented Feb 15, 2022

@viceice @ExNG Hello, GitLab team member here 👋

While multiple assignees on merge requests are a premium feature, this API is not a premium API. We check on the backend if the project has the appropriate license for multiple assignees. If it doesn't and multiple assignees are provided, then the MR is assigned to the first person and the rest are ignored.

https://gitlab.com/api/v4/projects/:project_id/merge_requests/42?assignee_ids[]=123 and https://gitlab.com/api/v4/projects/:project_id/merge_requests/42?assignee_id=123 are functionally identical, even if the project is on free tier. In fact, the assignee_id param actually gets converted to the array param on the backend.

You should be able to do this without the fallback and it will work fine.

Also, a few notes regarding licensing:

  • Licenses are applied on a per-namespace basis. A license belongs to a group, and applies to the projects under that group. If I have a premium license on my personal namespace (https://gitlab.com/bwill), then the license will only apply to projects under https://gitlab.com/bwill/:project. If I go work on a project that belongs to a different group that doesn't have a license, then I won't be able to use licensed features.
  • Self-managed GitLab instances can have a premium license applied to the entire instance, but this sort of check would not work for gitlab.com
  • I don't think we currently provide a means of checking for licensed features. Most users know what license their group has, so they don't need to check, but for integrations serving the general community this is definitely useful! I've created https://gitlab.com/gitlab-org/gitlab/-/issues/352843 to see about adding a means of checking licensed features to the API.

@viceice
Copy link
Member

viceice commented Feb 15, 2022

@Brcrwilliams thanks for explaining. Is there any version requirement when that feature was implemented?

@ExNG Can you please adapt the required changes.

@ExNG
Copy link
Contributor Author

ExNG commented Feb 15, 2022

Thanks @Brcrwilliams for your enlightenment :D

@viceice yeah i will do that since this not only fixes our problem but also simplifies the methods logic

@Brcrwilliams
Copy link

Brcrwilliams commented Feb 15, 2022

@viceice thanks for explaining. Is there any version requirement when that feature was implemented?

Multiple merge request assignees has existed for a long time. It looks like it became a premium feature in GitLab 13.9 (seems it may have been a core feature before that?). There was a bug fixed in GitLab 10.4 where merge requests wouldn't be updated if assignee_id was empty, but that GitLab version is quite old and no longer in-support.

@Taucher2003
Copy link

It looks like it became a premium feature in GitLab 13.9 (seems it may have been a core feature before that?).

That is just a documentation thing to remove references to the Starter Plan, which is now only used for the customers, which had it before removal.


I think, most people using Renovate on GitLab use a scheduled CI Pipeline for that. They would be covered by the $GITLAB_FEATURES environment variable, which contains all available features. However, that does not solve the problem for users, which host Renovate in another way.

@ExNG ExNG requested a review from viceice February 15, 2022 14:23
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Please add a note to the platform readme.md about the license requirements for multiple assignees.

@ExNG ExNG requested a review from viceice February 16, 2022 09:28
rarkins
rarkins previously approved these changes Feb 16, 2022
@rarkins rarkins requested a review from HonkingGoose February 16, 2022 09:45
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@viceice viceice merged commit 59f98ea into renovatebot:main Feb 16, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 31.84.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants