Skip to content

fix: remove project.approvals.set_approvals() method#2333

Merged
nejch merged 2 commits intomainfrom
jlvillal/remove_approvers_endpoint
Oct 19, 2022
Merged

fix: remove project.approvals.set_approvals() method#2333
nejch merged 2 commits intomainfrom
jlvillal/remove_approvers_endpoint

Conversation

@JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Oct 19, 2022 •
edited
Loading

The project.approvals.set_approvals() method used the
/projects/:id/approvers end point. That end point was removed from
GitLab in the 13.11 release, on 2-Apr-2021 in commit
27dc2f2fe81249bbdc25f7bd8fe799752aac05e6 via merge commit
e482597a8cf1bae8e27abd6774b684fb90491835. It was deprecated on
19-Aug-2019.

See merge request:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57473

@JohnVillalovos JohnVillalovos requested a review from nejch October 19, 2022 06:04
nejch
nejch requested changes Oct 19, 2022
View reviewed changes
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 @JohnVillalovos - was gonna get to this myself but you beat me to it 😅

I think we can now also remove/modify this part and potentially start testing approvals in functional tests:

python-gitlab/tests/functional/api/test_merge_requests.py

Lines 132 to 134 in dd04e8e

project.approvals.set_approvers([1], [])
approval = project.approvals.get()
assert approval.approvers[0]["user"]["id"] == 1

However, our project-level approval managers/objects are missing some mixing potentially.
Otherwise I just had some notes on the difference no project/MR-level approvals.

@JohnVillalovos JohnVillalovos marked this pull request as draft October 19, 2022 14:41
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/remove_approvers_endpoint branch from 99d27fe to 1a5b47c Compare October 19, 2022 16:02
@JohnVillalovos JohnVillalovos marked this pull request as ready for review October 19, 2022 16:02
@JohnVillalovos JohnVillalovos requested a review from nejch October 19, 2022 16:02
@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Oct 19, 2022

Thanks @JohnVillalovos - was gonna get to this myself but you beat me to it 😅

I think we can now also remove/modify this part and potentially start testing approvals in functional tests:

python-gitlab/tests/functional/api/test_merge_requests.py

Lines 132 to 134 in dd04e8e

project.approvals.set_approvers([1], [])
approval = project.approvals.get()
assert approval.approvers[0]["user"]["id"] == 1

I agree we should get the tests working. But I just wanted to remove the non-working endpoint at this stage. Getting the tests working is a separate issue and MR in my mind.

@nejch
Copy link
Member

nejch commented Oct 19, 2022

Thanks @JohnVillalovos - was gonna get to this myself but you beat me to it sweat_smile
I think we can now also remove/modify this part and potentially start testing approvals in functional tests:

python-gitlab/tests/functional/api/test_merge_requests.py

Lines 132 to 134 in dd04e8e

project.approvals.set_approvers([1], [])
approval = project.approvals.get()
assert approval.approvers[0]["user"]["id"] == 1

I agree we should get the tests working. But I just wanted to remove the non-working endpoint at this stage. Getting the tests working is a separate issue and MR in my mind.

Ok, makes sense. the xfail is a good indicator anyway that there's something wrong with the code and we should fix it at some point.

Could we maybe do a fix: remove dead code in .. or something like that so it shows up in the changelog? I wouldn't consider it a breaking change for something from 13.x but just if someone does still use it, they'll see immediately why it's gone in the release :)

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/remove_approvers_endpoint branch from 1a5b47c to 431ae7f Compare October 19, 2022 16:36
@JohnVillalovos JohnVillalovos changed the title chore: remove set_approvals() method fix: remove project.approvals.set_approvals() method Oct 19, 2022
@JohnVillalovos
fix: remove project.approvals.set_approvals() method
91f08f0 
The `project.approvals.set_approvals()` method used the
`/projects/:id/approvers` end point. That end point was removed from
GitLab in the 13.11 release, on 2-Apr-2021 in commit
27dc2f2fe81249bbdc25f7bd8fe799752aac05e6 via merge commit
e482597a8cf1bae8e27abd6774b684fb90491835. It was deprecated on
19-Aug-2019.

See merge request:
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57473
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/remove_approvers_endpoint branch from 431ae7f to 91f08f0 Compare October 19, 2022 16:37
@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Oct 19, 2022 •
edited
Loading

Could we maybe do a fix: remove dead code in .. or something like that so it shows up in the changelog? I wouldn't consider it a breaking change for something from 13.x but just if someone does still use it, they'll see immediately why it's gone in the release :)

Done! 🙂 Good idea!

@JohnVillalovos
chore: add not-callable to pylint ignore list
f0c02a5 
The `not-callable` error started showing up. Ignore this error as
it is invalid. Also `mypy` tests for these issues.

Closes: #2334
@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Oct 19, 2022

@nejch @lmilbaum I also put a fix for #2334 in this PR.

@lmilbaum
Copy link

lmilbaum commented Oct 19, 2022

@nejch @lmilbaum I also put a fix for #2334 in this PR.

Cool. Than, I can remove the astroid pinned version in #2320.

@nejch nejch merged commit eb54adf into main Oct 19, 2022
@nejch nejch deleted the jlvillal/remove_approvers_endpoint branch October 19, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nejch nejch nejch approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JohnVillalovos @nejch @lmilbaum