- Notifications
You must be signed in to change notification settings - Fork675
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c2c3882 to6e8d054Comparecodecovbot commentedJan 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown.Click here to find out more.
🚀 New features to boost your workflow:
|
6e8d054 to681168eComparenejch commentedJan 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 commentedFeb 7, 2025
@zapp42 would you be able to rebase this now? |
igorp-collabora commentedFeb 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@zapp42 after the#3083 the RESTManager is now generic and mixins are subclasses. This means the class definition instead of: Should be: |
zapp42 commentedFeb 7, 2025
@igorp-collabora I will give it a shot over the weekend. |
f338500 to1f5309eComparezapp42 commentedFeb 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 commentedFeb 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The RESTObject can have managers defined in its body. For example, Project object has a bunch of managers defined: python-gitlab/gitlab/v4/objects/projects.py Lines 175 to 190 in22be96c
|
zapp42 commentedFeb 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. |
bacda18 to40dbb29CompareSachinKSingh28 commentedFeb 14, 2025
This is great!@zapp42 would it be possible to add some documentation and functional tests? Happy to help support this. |
zapp42 commentedApr 9, 2025
@SachinKSingh28 Hi, sorry, I forgot to check for comments here. What do you need? |
40dbb29 tod071d9aCompared071d9a tocd9cfa8CompareThere 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.
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
ServiceAccountandServiceAccountManagerfor global service accounts - Add
GroupServiceAccountAccessTokenand its manager to handle group service account tokens - Update tests for creating/deleting service accounts and tokens, and expose
service_accountsin 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 | ImplementServiceAccount, managers, and group service account token classes |
| gitlab/client.py | Exposeservice_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.tokento ensure the token is correctly returned.
def test_create_group_service_account_access_token(| classServiceAccount(RESTObject): | ||
| pass | ||
| classServiceAccountManager(CreateMixin[ServiceAccount],ListMixin[ServiceAccount]): |
CopilotAIJun 7, 2025
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.
ServiceAccount currently has no delete support; add ObjectDeleteMixin to the class and DeleteMixin to ServiceAccountManager so that instance-level service accounts can be deleted.
| classServiceAccount(RESTObject): | |
| pass | |
| classServiceAccountManager(CreateMixin[ServiceAccount],ListMixin[ServiceAccount]): | |
| classServiceAccount(ObjectDeleteMixin,RESTObject): | |
| pass | |
| classServiceAccountManager(CreateMixin[ServiceAccount],DeleteMixin[ServiceAccount],ListMixin[ServiceAccount]): |
syphernl commentedAug 28, 2025
It seems this PR has stalled. That's a pity because we are eagerly awaiting this functionality. Are there any plans on finishing this? |
Changes
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.