Skip to content

Service account improvements #3109

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zapp42
Copy link

@zapp42 zapp42 commented Jan 29, 2025

Changes

  • Add support for instance level service accounts
  • Add support for group service accounts access tokens

Documentation and testing

Note: I have done only very basic tests (creation) and also did not include tests. Sorry for that. If you don't want to merge it like this, maybe it can at least serve as a template for someone else.

@zapp42 zapp42 force-pushed the service-account-improvements branch from c2c3882 to 6e8d054 Compare January 29, 2025 09:13
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.33%. Comparing base (378a836) to head (cd9cfa8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3109   +/-   ##
=======================================
  Coverage   97.32%   97.33%           
=======================================
  Files          98       98           
  Lines        6057     6073   +16     
=======================================
+ Hits         5895     5911   +16     
  Misses        162      162           
Flag Coverage Δ
api_func_v4 83.61% <100.00%> (-0.13%) ⬇️
cli_func_v4 84.73% <100.00%> (+0.04%) ⬆️
unit 90.23% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
gitlab/client.py 98.73% <100.00%> (+<0.01%) ⬆️
gitlab/v4/objects/service_accounts.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zapp42 zapp42 force-pushed the service-account-improvements branch from 6e8d054 to 681168e Compare January 29, 2025 10:04
@nejch
Copy link
Member

nejch commented Jan 29, 2025

Thanks a lot for your contribution @zapp42! If you don't mind I'd wait until @igorp-collabora's #3083 is merged (it's a big one) to avoid too many rebases on the other PR.

Once that's merged I'll propose a few tiny changes here to align with the new typing.

Tests would be great (even if simple functional ones) but we can make a follow-up, no problem!

@nejch
Copy link
Member

nejch commented Feb 7, 2025

@zapp42 would you be able to rebase this now?

@igorp-collabora
Copy link
Contributor

igorp-collabora commented Feb 7, 2025

@zapp42 after the #3083 the RESTManager is now generic and mixins are subclasses. This means the class definition instead of:

class ServiceAccountManager(CreateMixin, ListMixin, RESTManager):

class GroupServiceAccountAccessTokenManager(CreateMixin, RotateMixin, RESTManager):

Should be:

class ServiceAccountManager(CreateMixin[ServiceAccount], ListMixin[ServiceAccount]):


class GroupServiceAccountAccessTokenManager(CreateMixin[GroupServiceAccountAccessToken], RotateMixin[GroupServiceAccountAccessToken]):

@zapp42
Copy link
Author

zapp42 commented Feb 7, 2025

@igorp-collabora I will give it a shot over the weekend.

@zapp42 zapp42 force-pushed the service-account-improvements branch 3 times, most recently from f338500 to 1f5309e Compare February 8, 2025 21:16
@zapp42
Copy link
Author

zapp42 commented Feb 8, 2025

Note: I struggled with the group service account access token api. I did not find another example in the code base where you have this kind of nested objects (group -> service_account -> access_token). It is currently not hooked up in the API, but it works in the cli tool.

A hint for adding the group service account access token api is appreciated.

@igorp-collabora
Copy link
Contributor

igorp-collabora commented Feb 9, 2025

Note: I struggled with the group service account access token api. I did not find another example in the code base where you have this kind of nested objects (group -> service_account -> access_token).

The RESTObject can have managers defined in its body. For example, Project object has a bunch of managers defined:

class Project(
RefreshMixin, SaveMixin, ObjectDeleteMixin, RepositoryMixin, UploadMixin, RESTObject
):
_repr_attr = "path_with_namespace"
_upload_path = "/projects/{id}/uploads"
access_tokens: ProjectAccessTokenManager
accessrequests: ProjectAccessRequestManager
additionalstatistics: ProjectAdditionalStatisticsManager
approvalrules: ProjectApprovalRuleManager
approvals: ProjectApprovalManager
artifacts: ProjectArtifactManager
audit_events: ProjectAuditEventManager
badges: ProjectBadgeManager
boards: ProjectBoardManager
branches: ProjectBranchManager

@zapp42
Copy link
Author

zapp42 commented Feb 10, 2025

@igorp-collabora Thank you. I actually tried this before, but I messed up the _from_parent_attrs definition. Now it works as expected.

@zapp42 zapp42 force-pushed the service-account-improvements branch from bacda18 to 40dbb29 Compare February 10, 2025 08:02
@SachinKSingh28
Copy link
Contributor

This is great! @zapp42 would it be possible to add some documentation and functional tests? Happy to help support this.

@zapp42
Copy link
Author

zapp42 commented Apr 9, 2025

@SachinKSingh28 Hi, sorry, I forgot to check for comments here. What do you need?

@JohnVillalovos JohnVillalovos force-pushed the service-account-improvements branch from 40dbb29 to d071d9a Compare April 17, 2025 16:26
@JohnVillalovos JohnVillalovos force-pushed the service-account-improvements branch from d071d9a to cd9cfa8 Compare June 7, 2025 15:09
@JohnVillalovos JohnVillalovos requested a review from Copilot June 7, 2025 15:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for instance-level service accounts and group-level service account access tokens, along with basic tests and client exposure.

  • Introduce ServiceAccount and ServiceAccountManager for global service accounts
  • Add GroupServiceAccountAccessToken and its manager to handle group service account tokens
  • Update tests for creating/deleting service accounts and tokens, and expose service_accounts in the client

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/unit/objects/test_service_accounts.py Add unit tests for creating instance-level service accounts
tests/unit/objects/test_groups.py Add fixtures and tests for group service accounts and access tokens
gitlab/v4/objects/service_accounts.py Implement ServiceAccount, managers, and group service account token classes
gitlab/client.py Expose service_accounts manager in the GitLab client
Comments suppressed due to low confidence (3)

gitlab/v4/objects/service_accounts.py:53

  • GroupServiceAccountManager is missing ListMixin, preventing listing group service accounts; include ListMixin[GroupServiceAccount] in the inheritance list.
class GroupServiceAccountManager(

tests/unit/objects/test_groups.py:545

  • The deletion test has no assertion to verify the DELETE call succeeded; consider adding an assertion (e.g., checking that listing no longer returns the deleted account).
def test_delete_group_service_account(group, resp_delete_group_service_account):

tests/unit/objects/test_groups.py:552

  • After creating an access token, the test does not assert the returned token value; add an assertion for access_token.token to ensure the token is correctly returned.
def test_create_group_service_account_access_token(

Comment on lines +39 to +43
class ServiceAccount(RESTObject):
pass


class ServiceAccountManager(CreateMixin[ServiceAccount], ListMixin[ServiceAccount]):
Copy link
Preview

Copilot AI Jun 7, 2025

Choose a reason for hiding this comment

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

ServiceAccount currently has no delete support; add ObjectDeleteMixin to the class and DeleteMixin to ServiceAccountManager so that instance-level service accounts can be deleted.

Suggested change
class ServiceAccount(RESTObject):
pass
class ServiceAccountManager(CreateMixin[ServiceAccount], ListMixin[ServiceAccount]):
class ServiceAccount(ObjectDeleteMixin, RESTObject):
pass
class ServiceAccountManager(CreateMixin[ServiceAccount], DeleteMixin[ServiceAccount], ListMixin[ServiceAccount]):

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants