-
Notifications
You must be signed in to change notification settings - Fork 643
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
fix(api): fix saving merge request approval rules #2773
fix(api): fix saving merge request approval rules #2773
Conversation
7326a77
to
a168c63
Compare
FYI: The functional tests run against a GitLab EE instance. So tests should be feasible. |
a168c63
to
56266d8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2773 +/- ##
==========================================
- Coverage 96.52% 96.48% -0.05%
==========================================
Files 90 90
Lines 5872 5862 -10
==========================================
- Hits 5668 5656 -12
- Misses 204 206 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
56266d8
to
afc0ef8
Compare
I at least changed the unit test to have different ids for different entities, so that a mixup in ids would become noticable. |
What's the plan with this fix? How can I help to finish it? |
I consider this ready to be merged. @JohnVillalovos suggested adding functional tests, but I updated the unit tests instead to test this functionality. Is that sufficient? @david-vana-conrad, perhaps you can test this and confirm that it works correctly, if you have access to a Gitlab Ultimate instance. |
@Sjord Your fix works correctly on our GitLab Ultimate instance. Script has changed the Merge Request approval settings as requested. |
I've verified that this patch gets our .save() calls to gitlab.com hosted projects working again as well, would like to see it merged so we don't have to run a patched build to support our needs. :) |
afc0ef8
to
0866a12
Compare
The original bug was that the merge request identifier was used instead of the approval rule identifier. The test didn't notice that because it used `1` for all identifiers. Make these identifiers different so that a mixup will become apparent.
0866a12
to
d0c7fcd
Compare
Closes #2548
This seems to work when testing against a GitLab Ultimate test project. I assume that the integration tests run against a Gitlab CE instance, so adding a test for this is not feasible. I am not very sure of the changes I made, but it does seem to work correctly.
Changes
Remove custom code and correctly set identifiers for merge request approvals.
Documentation and testing
Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated: