Skip to content

feat: Add reset_approvals api #2279

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 1 commit into from
Sep 26, 2022

Conversation

zampierilucas
Copy link
Contributor

Added the newly added reset_approvals merge request api.
https://docs.gitlab.com/ee/api/merge_requests.html#reset-approvals-of-a-merge-request

Signed-off-by: Lucas Zampieri lzampier@redhat.com

@zampierilucas zampierilucas force-pushed the add_reset_approvals_api branch from d725e2c to 8a677b5 Compare September 22, 2022 19:53
@zampierilucas zampierilucas changed the title Add reset_approvals api feat: Add reset_approvals api Sep 22, 2022
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.

Thanks a lot @zampierilucas, looks great, I mainly just had a few style comments for docs.

I see you're getting a 404 on the functional test, did this work locally?

I've retried just in case, but I'm not sure if you need to first ensure that an approval exists for this to work, or if we maybe have an issue because this is done with an admin token and not a resource access token that this endpoint requires.

@nejch
Copy link
Member

nejch commented Sep 22, 2022

Oh I'm sorry @zampierilucas the only reason this is probably failing is that we're on an old GitLab image. Could you bump the docker image version here?
https://github.com/python-gitlab/python-gitlab/blob/main/tests/functional/fixtures/.env#L2

Something's wrong with our renovate setup, this was supposed to be auto-updated. After that's done, let's see if it works with an admin token or if we get a 401 as well in tests. Thanks again :)

@JohnVillalovos
Copy link
Member

Oh I'm sorry @zampierilucas the only reason this is probably failing is that we're on an old GitLab image. Could you bump the docker image version here? https://github.com/python-gitlab/python-gitlab/blob/main/tests/functional/fixtures/.env#L2

I have created a PR to update the Docker image: #2280

@zampierilucas zampierilucas force-pushed the add_reset_approvals_api branch from 8a677b5 to 2a14b20 Compare September 23, 2022 13:00
@zampierilucas
Copy link
Contributor Author

Thank you both @nejch and @JohnVillalovos for your feedback, I've applied @nejch suggestion to my last push, and rebase this branch on top of jlvillal/docker_image, so let's see if the pipeline passes now :D

@nejch nejch force-pushed the add_reset_approvals_api branch from 2a14b20 to 4408e76 Compare September 23, 2022 21:41
@nejch
Copy link
Member

nejch commented Sep 23, 2022

Thanks @zampierilucas and @JohnVillalovos! I merged the other PR and we can start fresh here.

So now we are hitting 401 as I suspected above because we run it as the root user. @zampierilucas two options:

  • You might need to create a project access token in that specific test case and override private_token, e.g. by creating a new Gitlab instance for that test
  • Alternatively you can write a unit test instead and just write some mocked fixtures.

I think adding the project access token will be quicker and would also test behavior against future gitlab versions so I think that might be nicer :) let us know if you can try and add that. See docs here: https://python-gitlab.readthedocs.io/en/stable/gl_objects/project_access_tokens.html#examples

Added the newly added reset_approvals merge request api.

Signed-off-by: Lucas Zampieri <lzampier@redhat.com>
@zampierilucas zampierilucas force-pushed the add_reset_approvals_api branch from 4408e76 to fc7b40c Compare September 26, 2022 17:48
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (b87a2bc) to head (fc7b40c).
Report is 657 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2279   +/-   ##
=======================================
  Coverage   95.28%   95.29%           
=======================================
  Files          79       79           
  Lines        5238     5246    +8     
=======================================
+ Hits         4991     4999    +8     
  Misses        247      247           
Flag Coverage Δ
api_func_v4 82.57% <100.00%> (+0.02%) ⬆️
cli_func_v4 82.52% <62.50%> (+0.08%) ⬆️
unit 87.62% <62.50%> (-0.04%) ⬇️

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

Files with missing lines Coverage Δ
gitlab/exceptions.py 100.00% <100.00%> (ø)
gitlab/v4/objects/merge_requests.py 85.51% <100.00%> (+0.62%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Thanks for the quick rework @zampierilucas! :) This should be available with the next release on the 28th.

@nejch nejch merged commit 88693ff into python-gitlab:main Sep 26, 2022
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.

4 participants