Add shared_runners_minutes_limit to groups and users API

What does this MR do?

Add shared_runners_minutes_limit to groups and users API

Are there points in the code the reviewer needs to double check?

• I am not satisfied with the use of @declared_params, but I don't want to list everything either.
• There's a few stuffs are EE-only. In order to make it conflict less, we might need to make some updates to CE first.
• accepts_nested_attributes_for :namespace needs the id to update the relation, rather than creating it. This is annoying. Not sure if we should also move this to CE, considering that we might want to add some settings for the namespace in CE, too.
• Do we really want to expose the namespace? Or just make it one of the field for user?

Why was this MR needed?

From #1909 (closed)

We need the ability to change this value through the API, so we can pilot this from customers.gitlab.com, and enable the right limits after a customer purchases or changes a plan.

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• Tests
  • Added for this feature/bug

What are the relevant issue numbers?

#1909 (closed)

@zj Please review for me, thanks! I listed my concerns at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942#are-there-points-in-the-code-the-reviewer-needs-to-double-check

• There's a few stuffs are EE-only. In order to make it conflict less, we might need to make some updates to CE first.

I don't see much problems here

• accepts_nested_attributes_for :namespace needs the id to update the relation, rather than creating it. This is annoying. Not sure if we should also move this to CE, considering that we might want to add some settings for the namespace in CE, too.

I agree

Do we really want to expose the namespace? Or just make it one of the field for user?

IMO exposing is fine

mentioned in issue #2458

assigned to @godfat

added 1 commit

• 2b71995e - Add shared_runners_minutes_limit to groups and users API

Compare with previous version

@grzesiek Could you please let me know what do you think about this? If you're ok with it, I'll start moving the needed changes to CE so that we could proceed. Thanks!

assigned to @grzesiek

@godfat It looks nice after a brief review, but I didn't go into details. Can we ask a miniboss to check if first? @nick.thomas do you have time to review it? Thanks!

assigned to @nick.thomas

@grzesiek Great, thanks, and congratulations on being a maintainer now

added 1 commit

• 6a0a4dfe - Use delegate so that we set shared_runners_minutes_limit

Compare with previous version

@nick.thomas I pushed another commit to make it set that directly from the user. https://gitlab.com/gitlab-org/gitlab-ee/commit/6a0a4dfea2bf9c1d55a8409f2bf4a85d0f30ed14 What do you think?

assigned to @godfat

Thanks @godfat, a few questions.

mentioned in commit 0106d2f9

added 1 commit

• 0106d2f9 - There's little point to check having at least one

Compare with previous version

added 1 commit

• 32e3955b - Make sure only the admin could update shared_runners_minutes_limit

Compare with previous version

resolved all discussions

unmarked as a Work In Progress

changed the description

added 2 commits

• d88c4c12 - Test for group creation and remove duplicated test
• 308aa151 - Update doc and add changelog entry

Compare with previous version

marked the checklist item Changelog entry added, if necessary as completed

changed the description

marked the checklist item Documentation created/updated as completed

added 606 commits

• 308aa151...13d77735 - 605 commits from branch master
• 17091ed5 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit

Compare with previous version

@nick.thomas Please review again, thanks! I added changelog, docs, and tests for creating the records as well.

assigned to @nick.thomas

assigned to @godfat

Couple more things ^^

mentioned in commit 46af4ac6

added 17 commits

• 17091ed5...b26cfbec - 15 commits from branch master
• e7d8b477 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit
• 46af4ac6 - Use GroupDetail for POST /groups

Compare with previous version

mentioned in commit 631f7026

added 11 commits

• 46af4ac6...36b3e067 - 9 commits from branch master
• 93083a43 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit
• 631f7026 - Don't check for admin permission if we're not

Compare with previous version

@nick.thomas So since it's a bit tricky to move the permission check to the services (I believe we'll need to fix a few things along the way), I propose we do it in another merge request. What do you think? If you also feel that's ok, I'll create an issue describing it.

assigned to @nick.thomas

added 11 commits

• 46af4ac6...36b3e067 - 9 commits from branch master
• 93083a43 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit
• 631f7026 - Don't check for admin permission if we're not

Compare with previous version

resolved all discussions

@grzesiek can you review as maintainer?

assigned to @grzesiek

@godfat I wonder if we should introduce more EE:: modules / classes to be in agreement with https://gitlab.com/help/development/ee_features.md. What do you think?

@godfat Looks nice! I left a couple of notes.

assigned to @godfat

mentioned in commit 14af431a

added 505 commits

• 631f7026...cc3e7a47 - 503 commits from branch master
• b6834235 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit
• 14af431a - Add 3 delegation tests for user

Compare with previous version

mentioned in issue #2531 (closed)

added 1 commit

• 0ed08366 - Move EE specific delegation to EE::User

Compare with previous version

assigned to @grzesiek

@godfat There still some unresolved comments in this MR!

assigned to @godfat

mentioned in issue #2553 (closed)

added 242 commits

• 0ed08366...eeb7245f - 240 commits from branch master
• 25b97e94 - Merge remote-tracking branch 'ee/master' into add-api-shared_runners_minutes_limit
• eaa4bfa7 - Fix updating shared_runners_minutes_limit; Check effects

Compare with previous version

@grzesiek @zj I updated all the tests added in this MR. I created an issue we could follow up. After this MR is accepted, I'll need to create a backport MR for various stuffs to reduce the chance of conflicts. It would be great if we could find a better way to do this, but I guess that's it for now. At least I tried to add "EE" comment everywhere to ease resolving conflicts. Perhaps we could try to move EE only tests to a separate directory first?

assigned to @grzesiek

mentioned in issue #2558 (closed)

@godfat I left just a one comment, we can address comments related to backporting changes to EE in a separate merge request. Let's merge this MR soon!

assigned to @godfat

mentioned in commit 9a44c9a6

added 1 commit

• 9a44c9a6 - Just check the value, simple and stupid

Compare with previous version

@grzesiek Thanks! There's still a lot to do :o Putting the check in service would also make it easier to extend for EE, we could use modules there. APIs in general needs some more love.

assigned to @grzesiek

added 1 commit

• c14bc19d - I think Rubocop Style/MultilineOperationIndentation is broken

Compare with previous version

@grzesiek We should probably fix the cop, or disable it. I don't think the cop really knows what it is talking about... https://gitlab.com/gitlab-org/gitlab-ee/commit/c14bc19d11ba8dc034e9428652d664a87f782255 I've hit into this many times already.

resolved all discussions

mentioned in issue #2563 (closed)

approved this merge request

Thanks @godfat! Looks good to me!

mentioned in commit 970f41cf

merged

mentioned in issue #1909 (closed)

@godfat @grzesiek Seeing how we'll use this API on https://gitlab.com/gitlab-com/customers-gitlab-com/issues/124, and given we've introduced a PUT /namespaces/:id admin API on https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1961/diffs#92a91e7ed2e63a4e6ef1a8591619c651765f7404_22_23, I'm wondering if we should keep the changes made on this MR - or just get rid of the introduced PUT /namespaces/:id. Note that both the Namespace#plan and Namespace#shared_runners_minutes_limit (data we'll need to update) for Groups and Users may be kept on a single Namespace admin endpoint.

I'm slightly inclined towards keeping it on a single admin Namespace endpoint, but please let me know your thoughts.

cc @DouweM

@oswaldo It might also be useful for users to use the API to find out the shared runner minutes limit for their user or group, which makes this API a little bit more user friendly. The same can be said for plan.

@oswaldo I have the same feeling as @DouweM. I could see that having only one API would be simpler and less confusing, but having both would be more user friendly because this could be viewed in different ways, and people might not know what is a namespace at all.

I would lean toward having both, but I don't strongly feel about it.

Agreed. Let's have both for now, thanks!