|
|
||
| Get a projec's CI/CD job token inbound allowlist:: | ||
|
|
||
| allowlist = scope.allowlist.list() |
There was a problem hiding this comment.
Not sure yet about this naming here, but it follows both the endpoint URL and our convention 😕
Sorry, something went wrong.
There was a problem hiding this comment.
yeah it would definetly be clearer if it were scope.projects_allowlist since gitlab have added groups_allowlist https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-allowlist-of-groups
But scope.allowlist does absolutely match the API design doc so it is probably best overall to be consistent with their docs 🤷
Sorry, something went wrong.
| try: | ||
| return cast(int, getattr(self, self._id_attr)) | ||
| except AttributeError: | ||
| return cast(int, getattr(self, "id")) |
There was a problem hiding this comment.
GitLab at it again - create response:
list response:
Sorry, something went wrong.
There was a problem hiding this comment.
I'm kind of thinking for the create to not return anything. As the caller appears to use all the values when creating that will be returned. Not sure if that would simplify things or not.
Sorry, something went wrong.
There was a problem hiding this comment.
Convention in REST and so far in python-gitlab would be to return the object that has been created. Means that in future if Gitlab add optional things into the contract which get returned back in the POST, user's would get insight into that
Sorry, something went wrong.
Codecov ReportAttention: 4 lines in your changes are missing coverage. Please review. Comparison is base (72e1aa7) 88.25% compared to head (7b0b22d) 96.42%. Additional details and impacted files@@ Coverage Diff @@
## main #2767 +/- ##
==========================================
+ Coverage 88.25% 96.42% +8.17%
==========================================
Files 90 90
Lines 5866 5880 +14
==========================================
+ Hits 5177 5670 +493
+ Misses 689 210 -479
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
|
This currently assumes a job token scope as the parent of the allowlist, but I don't think that is correct. I would expect the allowlist to work without retrieving the job token scope first: # Works, retrieves job_token_scope:
print(project.job_token_scope.get().allowlist.list())
# Doesn't work without job_token_scope:
print(project.job_token_scope.allowlist.list())
Test could look something like this: """
GitLab API: https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-inbound-allowlist
"""
import pytest
import responses
from gitlab.v4.objects.job_token_scope import AllowlistedProjectManager
allowlist_small = {"source_project_id":1,"target_project_id":2}
allowlist_large = {
"id": 2,
"description": None,
"name": "testproject",
"name_with_namespace": "testgroup / testproject",
"path": "testproject",
"path_with_namespace": "testgroup/testproject",
"created_at": "2024-01-21T19:18:38.829Z",
"default_branch": "main",
"tag_list": [],
"topics": [],
"ssh_url_to_repo": "git@localhost:testgroup/testproject.git",
"http_url_to_repo": "http://localhost/testgroup/testproject.git",
"web_url": "http://localhost/testgroup/testproject",
"readme_url": "http://localhost/testgroup/testproject/-/blob/main/README.md",
"forks_count": 0,
"avatar_url": None,
"star_count": 0,
"last_activity_at": "2024-01-23T18:22:16.321Z",
"namespace": {
"id": 3,
"name": "testgroup",
"path": "testgroup",
"kind": "group",
"full_path": "testgroup",
"parent_id": None,
"avatar_url": None,
"web_url": "http://localhost/groups/testgroup",
},
}
@pytest.fixture
def resp_get_allowlist():
with responses.RequestsMock(assert_all_requests_are_fired=False) as rsps:
rsps.add(
method=responses.GET,
url="http://localhost/api/v4/projects/1/job_token_scope/allowlist",
json=[allowlist_large],
content_type="application/json",
status=200,
)
yield rsps
@pytest.fixture
def resp_post_allowlist():
with responses.RequestsMock(assert_all_requests_are_fired=False) as rsps:
rsps.add(
method=responses.POST,
url="http://localhost/api/v4/projects/1/job_token_scope/allowlist",
json=allowlist_small,
content_type="application/json",
status=201,
)
yield rsps
@pytest.fixture
def allowlist(project):
return project.job_token_scope.allowlist
def test_get_allowlist(allowlist, resp_get_allowlist):
projects = allowlist.list()
assert projects[0].get_id() == 2
def test_create_allowlist(allowlist, resp_post_allowlist):
resp = allowlist.create({"target_project_id": 2})
assert resp.get_id() == 2
|
Sorry, something went wrong.
|
@Sjord very true, I would at least want to add support for lazy=True for GetWithoutIDMixin first so that this wouldn't require 2 API calls. The reason I did it this way was I wanted to follow GitLab's REST API path segment hierarchy. But lately it's making less and less sense to me, so we could also flatten this potentially - I'm not sure, also part of why this is still in draft 😅 thanks for the feedback! |
Sorry, something went wrong.
|
@nejch FYI if you haven't seen: Gitlab have just merged an update to GraphQL endpoint to add support for adding groups to the CI_JOB_TOKEN allowlist: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143132 And hopefully will be adding the same support to the REST API: https://gitlab.com/gitlab-org/gitlab/-/issues/435903#note_1761348177 POST /projects/:id/job_token_scope/allowlist |
Sorry, something went wrong.
|
@nejch FYI if you haven't seen: Gitlab have just merged an update to GraphQL endpoint to add support for adding groups to the CI_JOB_TOKEN allowlist: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143132 And hopefully will be adding the same support to the REST API: https://gitlab.com/gitlab-org/gitlab/-/issues/435903#note_1761348177 POST /projects/:id/job_token_scope/allowlist Thanks @TimKnight-DWP! 🙇 I'm pretty sure they will, because their terraform provider relies on REST. I'll get back to this PR this week. |
Sorry, something went wrong.
|
Cool - let me know if there's anything I can do to help with this MR @nejch , it's something that's high on our agenda, adding this here and then using from gitlabform to manage our state I'll take a look through Gitlabs MR and see how they intend to handle "Groups" whether it's taking group id (consistent with how projects are added) or something else. And feedback here. |
Sorry, something went wrong.
|
Design from Gitlab if the pattern is followed from GraphQl, either a new or the existing allowlist endpoint will use source_project_id, target_group_id POST I suspect they will add target_group_id as a supported parameter in the body of the request, in GQL they created a new mutation AddGroup DELETE probably takes group id into the |
Sorry, something went wrong.
|
Group Allowlist REST API -> https://gitlab.com/gitlab-org/gitlab/-/merge_requests/145069 |
Sorry, something went wrong.
|
@Sjord very true, I would at least want to add support for lazy=True for GetWithoutIDMixin first so that this wouldn't require 2 API calls. The reason I did it this way was I wanted to follow GitLab's REST API path segment hierarchy. But lately it's making less and less sense to me, so we could also flatten this potentially - I'm not sure, also part of why this is still in draft 😅 thanks for the feedback! Sorry @Sjord this was already done in another PR as is, but I think we could look into adding a lazy get at least, so we don't require another API call to use the endpoint. |
Sorry, something went wrong.
Changes
Closes #2762.
Adds https://docs.gitlab.com/ee/api/project_job_token_scopes.html#get-a-projects-cicd-job-token-inbound-allowlist
Documentation and testing
Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated: