feat(api): add support for Gitlab Deploy Token API #1052

machine424 · 2020-03-19T23:01:40Z

There is a conflict between the required parameters of POST /projects/:id/deploy_tokens (and POST /groups/:id/deploy_tokens) as documented here : https://docs.gitlab.com/ce/api/deploy_tokens.html#create-a-project-deploy-token and required parameters that the API waits for: gitlab.com that is uses 12.9.0-pre 4b94245907d (As I write these lines) and the docker image gitlab/gitlab-ce:nightly require four parameters: name, expires_at, username and scopes.

Creating a deploy token from gitlab.com UI agrees with the Deploy Token API documentation: only the name and the scopes are required.

The managers I added require the four parameters to make tests pass with gitlab/gitlab-ce:nightly.

the documentation on the other hand reflects the provided Deploy Token API documentation.

I've created an issue to make this clear: https://gitlab.com/gitlab-org/gitlab/-/issues/211878

max-wittig

Thanks for the MR! 👍 Nicely done overall, just a couple of comments regarding docs and a small fix

docs/gl_objects/deploy_tokens.rst

gitlab/v4/objects.py

tools/build_test_env.sh

docs/gl_objects/deploy_tokens.rst

nejch

@machine424 I restarted the build as it is now passing on latest with 12.9 released 👍

Btw, your PR now also enables usage of deploy tokens via the CLI! :) If you like, you could add docs (in docs/cli.rst) and tests for this in tools/cli_test_v4.sh, similar to project/group variables:

python-gitlab/tools/cli_test_v4.sh

Lines 163 to 186 in 8173021

    
           # Test project variables 
        
           testcase "create project variable" ' 
        
               OUTPUT=$(GITLAB -v project-variable create --project-id $PROJECT_ID \ 
        
                   --key junk --value car) 
        
           ' 
        
           testcase "get project variable" ' 
        
               OUTPUT=$(GITLAB -v project-variable get --project-id $PROJECT_ID \ 
        
                   --key junk) 
        
           ' 
        
           testcase "update project variable" ' 
        
               OUTPUT=$(GITLAB -v project-variable update --project-id $PROJECT_ID \ 
        
                   --key junk --value bus) 
        
           ' 
        
           testcase "list project variable" ' 
        
               OUTPUT=$(GITLAB -v project-variable list --project-id $PROJECT_ID) 
        
           ' 
        
           testcase "delete project variable" ' 
        
               OUTPUT=$(GITLAB -v project-variable delete --project-id $PROJECT_ID \ 
        
                   --key junk) 
        
           '

For deploy tokens, this might look something like:

testcase "create project deploy token" '
    OUTPUT=$(GITLAB -v project-deploy-token create --project-id $PROJECT_ID \
        --name foo --username root --expires-at "2021-09-09" --scopes "read_registry")
    echo $OUTPUT | grep -q "name: foo"
'
testcase "list all deploy tokens" '
    OUTPUT=$(GITLAB -v deploy-token list)
    echo $OUTPUT | grep -q "gitlab+deploy-token"
'

..where you grep for whatever output the server response should include. If the custom bash test syntax is too weird then don't worry though, we can also add these later.

machine424 · 2020-03-23T21:00:39Z

Cool, yes of course, I’ll add them.
But first, we have some problems to clear up, I don’t think the description of the PR and my last comments were clear, so I’ll rephrase:
1- There is an incoherence (at least for me) between the Gitlab documentation and the API:
Here in the doc: https://docs.gitlab.com/ee/api/deploy_tokens.html#create-a-project-deploy-token they state that expires_at and username are not required to create a deploy token. BUT the API waits for them. I opened an Issue about this here: https://gitlab.com/gitlab-org/gitlab/-/issues/211878 and Gitlab folks recognised the bug and proposed a workaround: PROVIDE empty values for expires_at and username ==> should we mark expires_at and username as required in _create_attrs ? should we override create method for the two Managers to pass empty expires_at and username to the API if the user didn’t provide them ?

2- Another problem: the API doesn’t take provided username into consideration, it overrides it, see: https://gitlab.com/gitlab-org/gitlab/-/issues/211963 => it’s not of python-gitlab’s business, but we should warn the users, do something else ?

I added some tests here : e9814b2 to illustrate my sayings.

nejch · 2020-03-24T09:56:52Z

Cool, yes of course, I’ll add them.
But first, we have some problems to clear up, I don’t think the description of the PR and my last comments were clear, so I’ll rephrase:

Nice, thanks! Sorry I wasn't fully following the previous round on this but I'll chime in.

1- There is an incoherence (at least for me) between the Gitlab documentation and the API:
Here in the doc: https://docs.gitlab.com/ee/api/deploy_tokens.html#create-a-project-deploy-token they state that expires_at and username are not required to create a deploy token. BUT the API waits for them. I opened an Issue about this here: https://gitlab.com/gitlab-org/gitlab/-/issues/211878 and Gitlab folks recognised the bug and proposed a workaround: PROVIDE empty values for expires_at and username ==> should we mark expires_at and username as required in _create_attrs ? should we override create method for the two Managers to pass empty expires_at and username to the API if the user didn’t provide them ?

2- Another problem: the API doesn’t take provided username into consideration, it overrides it, see: https://gitlab.com/gitlab-org/gitlab/-/issues/211963 => it’s not of python-gitlab’s business, but we should warn the users, do something else ?

I added some tests here : e9814b2 to illustrate my sayings.

I agree the empty value workaround won't work for libraries. How about just documenting this as a warning in the docs with links to upstream issues (at least that's what gitlab does for their open issues) - for both the optional attributes being required, and the username having no effect?

And once it's fixed upstream the exact GitLab versions affected can be defined. The tests will just need to provide the optional args as well for now. As you said it's the API behavior and not the wrapper. I see go-gitlab added this recently and aren't handling it on their side.

This way you don't have additional logic or breaking it for specific gitlab versions if a create() override is done and removed at some point later when the upstream fix is done.

Hm: Looking at it a bit more, seems like the upstream fix for the optional args is just changing required to optional (here and here. I'll see if I can do a few lines of Ruby, maybe then it can already get into a patch release soonish :P

nejch · 2020-03-24T23:54:08Z

I've started an upstream PR for the optional params, let's see if that gets merged into the next patch release, then we might not need to worry about it here. Not quite sure yet what's going on with username being ignored though..

machine424 · 2020-03-26T02:47:40Z

Great.
I've found another incoherence while adjusting some tests: https://gitlab.com/gitlab-org/gitlab/-/issues/212523
I have the impression that I'm testing the API.

max-wittig

Seems like this API is totally broken. How did this ever pass review at GitLab?

Your MR looks fine. Just a small change

tools/cli_test_v4.sh

machine424 · 2020-03-28T23:53:20Z

@max-wittig , when the client serves the server ! I'll not add more tests, I need this feature, lool.
I followed @nejch and I submitted a MR to correct the username overriding: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28175, the third bug https://gitlab.com/gitlab-org/gitlab/-/issues/212523 will be fixed by a Gitlab folk, Steve.
Once the 12.10 is released, I'll enable the tests that are commented.

docs/gl_objects/deploy_tokens.rst

nejch · 2020-03-29T15:28:42Z

@max-wittig , when the client serves the server ! I'll not add more tests, I need this feature, lool.
I followed @nejch and I submitted a MR to correct the username overriding: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28175, the third bug https://gitlab.com/gitlab-org/gitlab/-/issues/212523 will be fixed by a Gitlab folk, Steve.
Once the 12.10 is released, I'll enable the tests that are commented.

Wow cool, I never got to that one, thanks for that :) I approved with the two suggestions as you mentioned, it seems both upstream MR's will be merged in time for 12.10.

Yeah, thanks for adding all the CLI/integration tests, that's really extensive 😁

machine424 · 2020-03-30T23:34:38Z

Wow cool, I never got to that one, thanks for that :) I approved with the two suggestions as you mentioned, it seems both upstream MR's will be merged in time for 12.10.

Yeah, thanks for adding all the CLI/integration tests, that's really extensive 😁

There're never that extensive, but I really don't want to open a new issue on Gitlab 😉

tools/cli_test_v4.sh

max-wittig

Thank you for implementing this! LGTM 👍

machine424 mentioned this pull request Mar 19, 2020

Implement project deploy token #583

Closed

machine424 force-pushed the deploy-tokens-support branch from ce8567d to 588e684 Compare March 20, 2020 08:17

max-wittig requested changes Mar 20, 2020

View reviewed changes

max-wittig assigned machine424 Mar 20, 2020

machine424 force-pushed the deploy-tokens-support branch 2 times, most recently from c8c78f4 to 7703743 Compare March 21, 2020 19:39

machine424 requested a review from max-wittig March 21, 2020 19:39

machine424 removed their assignment Mar 21, 2020

nejch mentioned this pull request Mar 21, 2020

chore: improve test environment for upcoming features #1054

Merged

nejch reviewed Mar 22, 2020

View reviewed changes

machine424 requested a review from nejch March 23, 2020 21:00

machine424 force-pushed the deploy-tokens-support branch from e9814b2 to 15ca555 Compare March 26, 2020 02:43

max-wittig requested changes Mar 26, 2020

View reviewed changes

tools/cli_test_v4.sh Outdated Show resolved Hide resolved

tools/cli_test_v4.sh Outdated Show resolved Hide resolved

machine424 force-pushed the deploy-tokens-support branch from 15ca555 to f9ffbd1 Compare March 28, 2020 23:32

nejch approved these changes Mar 29, 2020

View reviewed changes

docs/gl_objects/deploy_tokens.rst Outdated Show resolved Hide resolved

docs/gl_objects/deploy_tokens.rst Outdated Show resolved Hide resolved

machine424 force-pushed the deploy-tokens-support branch from f9ffbd1 to 60285d9 Compare March 30, 2020 23:26

machine424 requested a review from max-wittig March 30, 2020 23:35

max-wittig requested changes Apr 3, 2020

View reviewed changes

tools/cli_test_v4.sh Outdated Show resolved Hide resolved

max-wittig assigned machine424 Apr 3, 2020

feat(api): add support for Gitlab Deploy Token API

01de524

machine424 force-pushed the deploy-tokens-support branch from 60285d9 to 01de524 Compare April 6, 2020 18:52

machine424 requested a review from max-wittig April 6, 2020 18:58

max-wittig approved these changes Apr 7, 2020

View reviewed changes

max-wittig merged commit 5979750 into python-gitlab:master Apr 7, 2020

	# Test project variables
	testcase "create project variable" '
	OUTPUT=$(GITLAB -v project-variable create --project-id $PROJECT_ID \
	--key junk --value car)
	'

	testcase "get project variable" '
	OUTPUT=$(GITLAB -v project-variable get --project-id $PROJECT_ID \
	--key junk)
	'

	testcase "update project variable" '
	OUTPUT=$(GITLAB -v project-variable update --project-id $PROJECT_ID \
	--key junk --value bus)
	'

	testcase "list project variable" '
	OUTPUT=$(GITLAB -v project-variable list --project-id $PROJECT_ID)
	'

	testcase "delete project variable" '
	OUTPUT=$(GITLAB -v project-variable delete --project-id $PROJECT_ID \
	--key junk)
	'

feat(api): add support for Gitlab Deploy Token API #1052

feat(api): add support for Gitlab Deploy Token API #1052

Uh oh!

Conversation

machine424 commented Mar 19, 2020

Uh oh!

max-wittig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

machine424 commented Mar 23, 2020

Uh oh!

nejch commented Mar 24, 2020

Uh oh!

nejch commented Mar 24, 2020

Uh oh!

machine424 commented Mar 26, 2020

Uh oh!

max-wittig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

machine424 commented Mar 28, 2020

Uh oh!

Uh oh!

Uh oh!

nejch commented Mar 29, 2020

Uh oh!

machine424 commented Mar 30, 2020

Uh oh!

Uh oh!

max-wittig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!