Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ProjectMergeRequestApprovalRule.save() throws 404 #2548

Closed
jwbaker opened this issue Apr 17, 2023 · 4 comments · Fixed by #2773
Closed

ProjectMergeRequestApprovalRule.save() throws 404 #2548

jwbaker opened this issue Apr 17, 2023 · 4 comments · Fixed by #2773
Labels
bug EE Issues related to the enterprise version of GitLab

Comments

@jwbaker
Copy link

jwbaker commented Apr 17, 2023

Description of the problem, including code/CLI snippet

gl.project.get(proj_id).merge_requests.get(mr_iid).approval_rules.get(rule_id).save()

This example is an MVP example; actually making changes to the rule object before calling .save() doesn't change the behaviour

Expected Behavior

The function should succeed silently, returning None

Actual Behavior

gitlab.exceptions.GitlabUpdateError: 404: 404 Not found is thrown. Trying it with debug mode on, it appears as though the root cause of the issue is that when the CLI invokes /projects/:id/merge_requests/:merge_request_iid/approval_rules/:approval_rule_id in the API, :id (i.e. project ID) is passed where the URL expects :approval_rule_id, as can be seen from this debug output (anonymized to remove sensitive information)

>>> rule.save()
DEBUG:urllib3.connectionpool:Resetting dropped connection: mygitlab.example.com
send: b'PUT /api/v4/projects/93/merge_requests/1/approval_rules/93 HTTP/1.1\r\nHost: mygitlab.example.com\r\nUser-Agent: python-gitlab/3.14.0\r\nAccept-Encoding: gzip, deflate\r\nAccept: */*\r\nConnection: keep-alive\r\nPRIVATE-TOKEN: TOKEN\r\nContent-type: application/json\r\nContent-Length: 768\r\n\r\n'
send: b'{"id": "93", "merge_request_iid": "1", "approval_rule_id": 89, "name": "testrule", "approvals_required": 1, "users": ["{\\"id\\": 168, \\"username\\": \\"myusername\\", \\"name\\": \\"My Name\\", \\"state\\": \\"active\\", \\"avatar_url\\": \\"https://secure.gravatar.com/avatar/8306d9f17d1c91970c2447b61c7a9f29?s=80&d=identicon\\", \\"web_url\\": \\"https://mygitlab.example.com/myusername\\", \\"created_at\\": \\"2023-03-29T14:30:13.371Z\\", \\"bio\\": \\"\\", \\"location\\": null, \\"public_email\\": null, \\"skype\\": \\"\\", \\"linkedin\\": \\"\\", \\"twitter\\": \\"\\", \\"website_url\\": \\"\\", \\"organization\\": null, \\"job_title\\": \\"\\", \\"pronouns\\": null, \\"bot\\": false, \\"work_information\\": null, \\"followers\\": 0, \\"following\\": 0, \\"is_followed\\": false, \\"local_time\\": null}"]}'
reply: 'HTTP/1.1 404 Not Found\r\n'

Specifications

  • python-gitlab version: 3.14.0
  • API version you are using (v3/v4): v4
  • Gitlab server version (or gitlab.com): 15.7.2-ee
@nejch nejch added bug EE Issues related to the enterprise version of GitLab labels Apr 17, 2023
@nejch
Copy link
Member

nejch commented Apr 17, 2023

Looks like we have some old code here:

https://github.com/python-gitlab/python-gitlab/blame/eb54adf2fe7d3c68dcb6021065e51ba33b7bbc04/gitlab/v4/objects/merge_request_approvals.py#L155

We might be able to just get rid of that custom save method, things might be more consistent these days. I don't use Premium features personally, so this needs some digging on why we even have that in the code.

Sjord added a commit to Sjord/python-gitlab that referenced this issue Jan 22, 2024
@Sjord
fix(api): fix saving merge request approval rules
acab43c 
Closes python-gitlab#2548
@Sjord Sjord mentioned this issue Jan 22, 2024
Merged
2 tasks
Sjord added a commit to Sjord/python-gitlab that referenced this issue Jan 22, 2024
@Sjord
fix(api): fix saving merge request approval rules
7326a77 
Closes python-gitlab#2548
Sjord added a commit to Sjord/python-gitlab that referenced this issue Jan 22, 2024
@Sjord
fix(api): fix saving merge request approval rules
a168c63 
Closes python-gitlab#2548
@Sjord
Copy link
Contributor

Sjord commented Jan 22, 2024

Indeed the used identifier is incorrect. It would perform a call to /api/v4/projects/1/merge_requests/1/approval_rules/3 instead of /api/v4/projects/1/merge_requests/1/approval_rules/1 when saving.

The unit test here does not catch that, since the project ID, merge request ID, and approval rule ID are all 1, so a mix up is not detected.

Docs for this feature:

JohnVillalovos pushed a commit to Sjord/python-gitlab that referenced this issue Jan 28, 2024
@Sjord @JohnVillalovos
fix(api): fix saving merge request approval rules
56266d8 
Closes python-gitlab#2548
Sjord added a commit to Sjord/python-gitlab that referenced this issue Jan 30, 2024
@Sjord
fix(api): fix saving merge request approval rules
7e80daa 
Closes python-gitlab#2548
@jarodwilson
Copy link
Contributor

jarodwilson commented May 13, 2024 •
edited

We're actively encountering this bug ourselves, was there a plan to get #2773 merged?

JohnVillalovos pushed a commit to Sjord/python-gitlab that referenced this issue May 13, 2024
@Sjord @JohnVillalovos
fix(api): fix saving merge request approval rules
7daa18f 
Closes python-gitlab#2548
JohnVillalovos pushed a commit to Sjord/python-gitlab that referenced this issue May 13, 2024
@Sjord @JohnVillalovos
fix(api): fix saving merge request approval rules
7e314a1 
Closes python-gitlab#2548
JohnVillalovos pushed a commit that referenced this issue May 13, 2024
@Sjord @JohnVillalovos
fix(api): fix saving merge request approval rules
b8b3849 
Closes #2548
@JohnVillalovos
Copy link
Member

We're actively encountering this bug ourselves, was there a plan to get #2773 merged?

It is now merged and a new release of python-gitlab was made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug EE Issues related to the enterprise version of GitLab
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(api): fix saving merge request approval rules Sjord/python-gitlab
5 participants
@Sjord @jarodwilson @JohnVillalovos @jwbaker @nejch