NotificationsYou must be signed in to change notification settings
Fork675
Star2.4k

fix(api): delete invalid 'project-runner get' command#1628

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

Jump to bottom

Merged

nejch merged 3 commits intopython-gitlab:mainfromlgatellier:fix/delete-project-runner-get

Oct 17, 2021

Merged

fix(api): delete invalid 'project-runner get' command#1628

nejch merged 3 commits intopython-gitlab:mainfromlgatellier:fix/delete-project-runner-get

Oct 17, 2021

Conversation

Copy link

Contributor

lgatellier commentedOct 9, 2021•
edited by nejch
Loading

Deletes the 'project-runner get' command which does'nt match any existing Gitlab API resource.

Closes#1608.

Copy link

codecov-commenter commentedOct 9, 2021•
edited
Loading

Codecov Report

Merging#1628 (d989c9b) intomain (4170dbe) willnot change coverage.
The diff coverage is100.00%.

@@           Coverage Diff           @@##             main    #1628   +/-   ##=======================================  Coverage   91.92%   91.92%           =======================================  Files          74       74             Lines        4295     4295           =======================================  Hits         3948     3948             Misses        347      347

Flag	Coverage Δ
cli_func_v4	`81.93% <100.00%> (ø)`
py_func_v4	`80.95% <100.00%> (ø)`
unit	`83.74% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
gitlab/v4/objects/runners.py	`98.18% <100.00%> (ø)`

nejch approved these changes

Oct 9, 2021

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

Thank you very much@lgatellier!

You are right that the group runner should only inherit fromListMixin. Whether you'd like to add this here - or open a new PR - is up to you :)

nejch reviewed

Oct 9, 2021

View reviewed changes

gitlab/v4/objects/runners.pyShow resolvedHide resolved

lgatellier force-pushed thefix/delete-project-runner-get branch from3514b2b to06ef25dCompare

October 9, 2021 12:47

Copy link

Member

nejch commentedOct 9, 2021

Funny, looks like there were actually some unit tests written that mocked POST/DELETE responses to the group runner. I'll check if this might be an undocumented API in gitlab already..

Copy link

Member

nejch commentedOct 9, 2021

Funny, looks like there were actually some unit tests written that mocked POST/DELETE responses to the group runner. I'll check if this might be an undocumented API in gitlab already..

@lgatellier I think you can safely delete the following to make it pass:

https://github.com/python-gitlab/python-gitlab/blob/master/tests/unit/objects/test_runners.py#L255-L256

and

https://github.com/python-gitlab/python-gitlab/blob/master/tests/unit/objects/test_runners.py#L266-L270

as real GitLab servers would not work with this.

Copy link

ContributorAuthor

lgatellier commentedOct 10, 2021

I removed the 2 test cases.
I think mocked responses related to enabling/disabling runner should be changed to remove "groups" from URI pattern.
Can you confirm that ?

@pytest.fixturedefresp_runner_enable():withresponses.RequestsMock()asrsps:pattern=re.compile(r".*?(projects|groups)/1/runners")@pytest.fixturedefresp_runner_disable():withresponses.RequestsMock()asrsps:pattern=re.compile(r".*?/(groups|projects)/1/runners/6")

Copy link

Member

nejch commentedOct 10, 2021

I removed the 2 test cases. I think mocked responses related to enabling/disabling runner should be changed to remove "groups" from URI pattern. Can you confirm that ?
@pytest.fixturedefresp_runner_enable():withresponses.RequestsMock()asrsps:pattern=re.compile(r".*?(projects|groups)/1/runners")@pytest.fixturedefresp_runner_disable():withresponses.RequestsMock()asrsps:pattern=re.compile(r".*?/(groups|projects)/1/runners/6")