Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 17, 2021

Conversation

lgatellier
Copy link
Contributor

@lgatellier lgatellier commented Oct 9, 2021

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

Closes #1608.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2021

Codecov Report

Merging #1628 (d989c9b) into main (4170dbe) will not change coverage.
The diff coverage is 100.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%> (ø)

Copy link
Member

@nejch 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 from ListMixin. Whether you'd like to add this here - or open a new PR - is up to you :)

@lgatellier lgatellier force-pushed the fix/delete-project-runner-get branch from 3514b2b to 06ef25d Compare October 9, 2021 12:47
@nejch
Copy link
Member

nejch commented Oct 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..

@nejch
Copy link
Member

nejch commented Oct 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.

@lgatellier
Copy link
Contributor Author

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.fixture
def resp_runner_enable():
    with responses.RequestsMock() as rsps:
        pattern = re.compile(r".*?(projects|groups)/1/runners")

@pytest.fixture
def resp_runner_disable():
    with responses.RequestsMock() as rsps:
        pattern = re.compile(r".*?/(groups|projects)/1/runners/6")

@nejch
Copy link
Member

nejch commented Oct 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.fixture
def resp_runner_enable():
    with responses.RequestsMock() as rsps:
        pattern = re.compile(r".*?(projects|groups)/1/runners")

@pytest.fixture
def resp_runner_disable():
    with responses.RequestsMock() as rsps:
        pattern = re.compile(r".*?/(groups|projects)/1/runners/6")

That's right @lgatellier, thank you!

@nejch
Copy link
Member

nejch commented Oct 14, 2021

@lgatellier would you mind rebasing this onto the main branch?

@lgatellier lgatellier force-pushed the fix/delete-project-runner-get branch from dffa8a3 to d989c9b Compare October 17, 2021 17:01
@lgatellier
Copy link
Contributor Author

@nejch all done, mocked responses are fixed, and branch is rebased !

@nejch nejch merged commit 905781b into python-gitlab:main Oct 17, 2021
@nejch
Copy link
Member

nejch commented Oct 17, 2021

Thanks again @lgatellier!

@lgatellier lgatellier deleted the fix/delete-project-runner-get branch October 17, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'project-runner get' command should not exist
3 participants