As noted in the docs:
Any merge requests that were created before changing the rules will not be changed. They will keep the original approval rules, unless manually overridden.
We are likely not applying approval rule updates to open merge requests currently due to performance reasons, but it may be the users expectation (pending workflowsolution validation) that updating the projects approval rules or changing the branch for the approval rules will affect existing merge requests.
Proposal
When updating the approval rules in the project settings, these changes should apply to existing merge requests that have not had their approval rules overridden.
@m_gill@katokpara@danielgruesso ok, so I went down the rabbit hole to find exactly why we implemented the current behavior: not updating existing merge requests when changing project approval rules.
The “Merge request approvers” feature was first introduced in 7.12, in July 2015 (blog post; docs). At that time, the only thing you could do was set the number of required approvals. Users were not able to override that project setting per MR.
In 7.13 we added the ability to set the default approvers (blog post; docs). The key here is default, because it was possible to override the approvers per MR. We still did not allow overriding the number of required approvals.
I tried to find the issues were these decisions were made, but to no avail.
If approver overriding is enabled and the project level approvers are changed after a merge request is created, the merge request retains the previous approvers.
With this last bit of information, I tried this out on GitLab.com:
If approvals overriding is disabled, changing the project approval rules changes all existing MRs.
If approvals overriding is enabled, changing the project approval rules does not change any of the existing MRs, regardless if those MRs have overridden the project rules or not.
Furthermore, if I override an MRs approval rules and then disable overriding, the overridden MR assumes the project approval rules. But if I then enable overriding again, the MR recovers the rules that I had overridden, and is back to how it was before overriding was disabled.
Conclusion: I'm not sure if this is intentional, but the behavior seems odd. I would expect that when overriding is enabled, changing project approval rules only updates existing MRs that haven't been overridden. When overriding is disabled we already the predictable behavior of updating all existing MRs when changing project approval rules, which seems correct to me.
Any merge requests that were created before changing the rules will not be changed. They will keep the original approval rules, unless manually overridden.
@katokpara yes, it's that first checkbox in Project Settings > General > Merge request approvals: Can override approvers and approvals required per merge request
I'm leaning more towards Code Review because we'll need to work on how the merge request responds to the approval rule I can go either way too, and I do not have a great grasp right now on the line between code review and source code when it comes to this - which will happen in time.
For 13.7 though, let's keep it with Code Review because they have freed up capacity, while we may not have worked on it originally? WDYT @phikai?
@pedroms if I'm reading your investigation correctly, the behavioral change that needs to be made here is "when approvals overriding is enabled, changing the project approval rules will update existing MRs rules where someone hasn't already made manual changes to them"
@kerrizor yes, that's it. Today we already update approval rules in existing MRs when overriding is disabled, so this would extend that behavior to also cover the case when overriding is enabled.
changing the project approval rules will update existing MRs rules where someone hasn't already made manual changes to them
Just to be clear, it should only update MRs that have a pristine set of approval rules. “Pristine” here is a set of rules that haven't been changed and match the project's default approval rules. Here's an example:
MR A: 4 rules, none were modified
MR B: 4 rules, 1 of the default project rules was modified
MR C: 2 rules, all default project rules were deleted and 2 new rules were manually created
When the project rules are updated, only MR A will be updated to have all of its approval rules match the project defaults.
Hi all! I was just chatting approval rules with Kerri and she brought this issue up. Does this conflict with (or at least change) the issue I'm working on? #205422 (closed)
Issue TLDR: Changing a project rule's target shouldn't change any merge request rules' targets since merge request rules shouldn't be updated by project rules.
is a bit of a "big rabbit" for the server to swallow, potentially updating thousands of rules.. and if modifications are made in rapid succession, we can be queuing up large amounts of pointless work, and have weird windows of mismatch between AMRRs and their APRs. I have a solution using this approach 50% complete, it needs an afternoon of making it an async worker
is neat, but getting snagged up in a handful of tests breaking due to implied creation of rules - AMRRs can apparently be created, associated with an APR, yet have different values than the APR without being marked as modified (since they are created with different values and then associated after the fact.) This might simply be the result of poor test setup, but might be an implication of an assumption deeper in the modeling that needs to be addressed (or acknowledged.) Assuming the issue is only naive testing, !48511 (closed) is 90% complete
After going over with reviewers, I feel we're at a bit of an impasse. Neither approach is really great - each has some subtle issues, and in fact, @patrickbajao came up and additional idea for an architectural change that I think is interesting to look at, but may take more work than we have left in %13.8 (or, at least, if we're interested in that refactor approach, we need to review what our timeline of delivering a solution to the root issue of this specific deliverable.)
!48511 (comment 477202833) outlines the proposal, but it boils down to changing it so that rather than having ApprovalRules act as templates, we instead change the ApprovalProjectRules and ApprovalMergeRequestRules to be not individual records, but relationships associating the ApprovalRule and the MergeRequest/Project. When a user modifies an ApprovalRule, we've nothing to update, and when a user creates a custom rule or change within the context of a specific MergeRequest, we create a new ApprovalRule and associate it to the MergeRquest (and "simply" disassociate any previously existing rules that are now "modified") It's a very subtle twist on the delegation approach I was working on in , but removes the need for mass-updating, as well as the fancy logic I was having to add via potentially confusing meta-programming.
It's a fair whack of work, however, so we should really hash it out and make sure we're comfortable with that direction before committing to it as a pivot. @m_gill@pedroms what's our relative "hurry up and ship a solution" of the root issue here? Can we wait on completing a deeper rearchitecting of ApprovalRules, pushing this into %13.9, or should we push ahead on shipping the current solution in !48511 (closed) and then circle back in %13.9 or %13.10 to address such an approach?
(Tangentially, I've opened #296565 (closed) to address some backend concerns caused by the current UI of the admin section for creating/modifying ApprovalRules that impact us regardless of our approach. I'd like to see get into %13.9 if possible.)
Thanks for bringing this up @kerrizor! I just want to add that one major part (aside from looking into edge cases) I see in the redesign I suggested is migration of data from old tables to new tables. That will likely take a milestone by itself (of course this is just a naive estimate).
@kerrizor I see that with !48511 (closed) also there are additional steps (like mitigating the UI, and testing for performance) that will need to happen before releasing - so it's not as simple as merging the existing MR versus pivoting. With this new approach, what are all the steps that need to happen? Does this impact our reviewers work (#233736 (closed) for example)? Maybe if we can get an outline of the steps that need to happen in order for proposal to move forward, and what effort those will be, we can see a realistic timeline for the proposal.
Thanks for the prompt, @m_gill.. I held off breaking it down because it was a bit late in my day when I wrote that, but let’s take a swing now.
I’m know for a fact that I’m missing some steps here, which I hope @patrickbajao can fill in. Especially around the wrapped rules and the ApprovalState - I’m assuming there’s complexity in there I’m not familiar enough with to conceptualize the work involved.
Complications/Questions/Thoughts
This should be an Epic
The usual disclaimer about edge cases I'm not accounting for
If we take an incremental approach, once we have parallel create/destroy routines and the data migrated (but not destroyed) we could put this behind a feature flag, for “oh no we forgot ____” panic.
How do we handle code owners? I think we get this for free just fine, but we should confirm.
How does this interact with ApprovalWrappedRules? Do we still need them?
Do we even needApprovalRulesProject, or just a field that indicated that it is a “project level” rule? That’d simplify our model immensely, wouldn’t it?
Create new tables and models
ApprovalRule
ApprovalRulesProject
ApprovalRulesMergeRequest
These would be join tables/models, so fairly thin.
We need to do some thinking here about what data from the existing architecture ends up where, since we lose the distinction between “Project” and “Merge Request” rules
I’m not sold on *_v2 here, but if we want to move somewhat iteratively, we might need to have both the new and the old relationships still active.. I’m not convinced, though.
Need to include the #applicable_to_branch scope on the ApprovalRulesMergeRequest class
Also need #inapplicable_to_branch scope for ApprovalRulesProject
Do we need to provide a mechanism for accessing the legacy rule records at all?
W:3 (there’s unknowns here)
Migrate existing Approval*Rule functionality to new classes
I want this to be straightforward, but it may take some poking around - self is no longer an ApprovalRule but the relationship record, so I think it’s probably just tediousness here.
W: 2 (optimistic)
Update existing ApprovalMergeRequestRule creation/destruction locations to also create/destory ApprovalRuleMergeRequest relationships
Depending on the timing, we could also remove the existing create/destroy of ApprovalMergeRequestRule records, but if we have to be incremental, we can just do the “piggybacking” here
W:2
Migrate existing Approval(MR/Project)Rules
When it is an unmodified rule, add the new relationship
When it is a modified rule, create it as a new ApprovalRule and associate with the MR
Our goal here is to have a table of ApprovalRules that are the existing project-level rules created by admins, as well as any custom rules created for the individual MRs.
W:3 - this is some lifting, I’m afraid.
Replace indices
The legacy tables have a number of indices applied to them, so we’ll need to inventory and re-apply any that are still relevant.. but on approval_rules
We need to update every place we interaction with Approval*Rules , and make sure they work correctly and/or refer to the correct record(s). This is a fairly diffuse task, so I’ll just sort of hand wave about it here.
W:2
Remove legacy relationships and replace with _v2 relationships
How does this interact with ApprovalWrappedRules? Do we still need them?
I imagine we'll still need it given that an ApprovalRule will have no direct access to a specific MergeRequest since the relation is many-to-many. It may change though.
Do we even needApprovalRulesProject, or just a field that indicated that it is a “project level” rule? That’d simplify our model immensely, wouldn’t it?
I really think we need it to know which ApprovalRules are associated to a certain Project. If we are going to use a single join table, we need polymorphic tables which we is considered antipattern in GitLab codebase.
Migrate existing Approval(MR/Project)Rules
This is going to be background migration highly likely. Since it's going to run asynchronously, we have to consider what will happen during the time that the data isn't migrated yet for a certain MergeRequest or Project.
Since it's going to be a background migration, we need to clean it up at some point. This needs to happen before "Remove legacy relationships and replace with _v2 relationships" I imagine.
In my thinking, an ApprovalRule would only ever be has_one :project so wouldn't need a join table?
Ahh yup, true that @kerrizor, thanks for pointing that out! We can make the belongs_to :project optional and set it to nil if it's a merge request rule.
Thanks for the summary @kerrizor. It looks like this issue has no weight... would you add a weight to it based on the effort you think is left for 13.8?
I am going to mark as slipped because it seems unlikely that this will be merged by tomorrow. Hopefully I am wrong and we can remove the label! But for now I would rather account for this in %13.8.
@pedroms@m_gill How real of a problem do we think this is vs. could we just be more explicit in the docs about the way things work? Here's why I ask:
Based on the investigation from Pedro this looks to be existing behavior for quite some time, yet this issue is relatively new and there aren't any users/customers complaining (that are linked here).
The investigation from Kerri seems to indicate we've got some work ahead of us in either direction we choose.
Counter Proposal
What if we didn't do anything, or just made a docs clarification on the existing behavior and left it all alone? We could continue to monitor the issue and come back to this if we think the behavior needs to change, but maybe we don't need to address it now.
If so, I think we need still to solve the inconsistency detailed in #205422 (closed) (that issue was abandoned in favor of this one, see this discussion) — changing an approval rule's target branch affects existing merge requests when overriding approval rules is enabled, which is a different behavior from all of the other approval rule fields.
If so, I think we need still to solve the inconsistency detailed in #205422 (closed) (that issue was abandoned in favor of this one, see this discussion) — changing an approval rule's target branch affects existing merge requests when overriding approval rules is enabled, which is a different behavior from all of the other approval rule fields.
@pedroms Couldn't we just update the docs for that too? I don't see any reason to dive in to these issues when we can clarify the current behavior in docs.
@phikai the reason to solve that inconsistency is because it can lead to confusion about how updating approval rules affects existing MRs, specifically because the target branch field behaves differently from every other aspect of approval rules. We could update the docs, but I don't think it's a reasonable thing to do when there are inconsistencies at this granular level. It's more likely for users to be confused and think it's a bug than going to the docs to see if this is a documented inconsistency.
I wish we didn't have to dive into these issues, but the feature was not implemented consistently in the first place. We shouldn't use the docs to avoid fixing bugs.
@pedroms Why don't we fix it if it's ever actually a problem. While we certainly can't say for sure that users don't encounter this issue, we do generally see support tickets and user generated issues when people are encountering some bugs. In the absence of those, and given the length of time it's been in product this is what we should do:
Close THIS issue as won't do. - we can always revisit if it's something we think we need to do in the future.
Create a NEW issue for documentation of the existing state of how approval rules respond. This way it's known how it works even if it's outside of expectation.
/cc @m_gill@kerrizor - Would one of you mind opening the documentation issue and MR, can we slot that in place of these issues?
@phikai I'll open up that issue/MR and work to get that in %13.8. I think the discussion @patrickbajao and I are having is interesting, so I'll open a new epic for that as well.
I am seeing this issue right now. - also sometimes existing MRs do get updated. this is inconsistent.
We had a specific person set as a required approver - which was removed.
The expectation was that the MRs will require 1 Approval (as this is the rule set to all branches)
In reality, anyone can merge now.. we have 23 MRs open.
I have a case where I am editing a project-level approval rule, to require 3 approvals instead of 1 for a particular branch, and expecting several existing open MR's to have their approval criteria updated to reflect this change. What is actually happening is that the approval criteria in the existing MR that I was monitoring is unchanged. I am now confused at what the expected behavior should be. Once a merge request has already been created, how immutable is it w.r.t changing project-level settings? Furthermore, I was unsuccessful at finding anything in the documentation that talks about this.