Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
zapp42 wants to merge6 commits intopython-gitlab:main
base:main
Choose a base branch
Loading
fromzapp42:service-account-improvements

Conversation

zapp42
Copy link

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.

@zapp42zapp42force-pushed theservice-account-improvements branch fromc2c3882 to6e8d054CompareJanuary 29, 2025 09:13
@codecovCodecov
Copy link

codecovbot commentedJan 29, 2025
edited
Loading

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
FlagCoverage Δ
api_func_v483.61% <100.00%> (-0.13%)⬇️
cli_func_v484.73% <100.00%> (+0.04%)⬆️
unit90.23% <100.00%> (+0.04%)⬆️

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

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

@zapp42zapp42force-pushed theservice-account-improvements branch from6e8d054 to681168eCompareJanuary 29, 2025 10:04
@nejch
Copy link
Member

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

@zapp42 would you be able to rebase this now?

@igorp-collabora
Copy link
Contributor

igorp-collabora commentedFeb 7, 2025
edited
Loading

@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

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

igorp-collabora and nejch reacted with thumbs up emoji

@zapp42zapp42force-pushed theservice-account-improvements branch 3 times, most recently fromf338500 to1f5309eCompareFebruary 8, 2025 21:16
@zapp42
Copy link
Author

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 commentedFeb 9, 2025
edited
Loading

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:

classProject(
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

nejch reacted with thumbs up emoji

@zapp42
Copy link
Author

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

@zapp42zapp42force-pushed theservice-account-improvements branch frombacda18 to40dbb29CompareFebruary 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

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

@JohnVillalovosJohnVillalovosforce-pushed theservice-account-improvements branch from40dbb29 tod071d9aCompareApril 17, 2025 16:26
@JohnVillalovosJohnVillalovosforce-pushed theservice-account-improvements branch fromd071d9a tocd9cfa8CompareJune 7, 2025 15:09
Copy link

@CopilotCopilotAI 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.

  • IntroduceServiceAccount andServiceAccountManager for global service accounts
  • AddGroupServiceAccountAccessToken and its manager to handle group service account tokens
  • Update tests for creating/deleting service accounts and tokens, and exposeservice_accounts in the client

Reviewed Changes

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

FileDescription
tests/unit/objects/test_service_accounts.pyAdd unit tests for creating instance-level service accounts
tests/unit/objects/test_groups.pyAdd fixtures and tests for group service accounts and access tokens
gitlab/v4/objects/service_accounts.pyImplementServiceAccount, managers, and group service account token classes
gitlab/client.pyExposeservice_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 foraccess_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

CopilotAIJun 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
classServiceAccount(RESTObject):
pass
classServiceAccountManager(CreateMixin[ServiceAccount],ListMixin[ServiceAccount]):
classServiceAccount(ObjectDeleteMixin,RESTObject):
pass
classServiceAccountManager(CreateMixin[ServiceAccount],DeleteMixin[ServiceAccount],ListMixin[ServiceAccount]):

Copilot uses AI. Check for mistakes.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@zapp42@nejch@igorp-collabora@SachinKSingh28

[8]ページ先頭

©2009-2025 Movatter.jp