In an effort to solve the issue of changes to ApprovalProjectRules not cascading to the associated, unmodified ApprovalMergeRequestRules (#254958 (closed)) we came up with 2 different approaches:
update AMRRs when the ApprovalProjectRule (APR) changes (Draft: !48511 (04060448))
have AMRRs delegate to their related APR unless the AMRR is modified (Draft: !48511 (closed))
However, neither approach is really great - each has some subtle issues, in either performance or how much complicated metaprogramming we need to do in order to end up at the expected results. In the discussion on various options, @patrickbajao suggested a rearchitecting of approval rules might be the best long-term approach. !48511 (comment 477202833) outlines the proposal, but it boils down to changing it so that
Rather than having ApprovalProjectRules acting as templates, we instead would add
ApprovalRule
ApprovalRulesProject - join table associating ApprovalRule to a Project
ApprovalRulesMergeRequest - join table associating ApprovalRule to a MergeRequest
Rather than having individual records then, we reference the parent rule through the relationship associating the ApprovalRule and the MergeRequest/Project. When a user modifies a project-level ApprovalRule, we've nothing to update, and the change automatically is applied to all open MRs. When a user creates a custom rule or changes an existing one 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 towards, but removes the need for mass-updating, as well as the fancy logic I was having to add via potentially confusing meta-programming.
I took an initial pass at breaking this apart. I 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 might be an Epic all its own
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. We do get this for free
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? We don't!
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
Do we need to provide a mechanism for accessing the legacy rule records at all?
W:3 (there’s unknowns here)
Migrate existing ApprovalMergeRequestRule 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 ApprovalMergeRequestRule , 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
Awesome work, Kerri I think this looks like a solid plan!
Just poking my head in for a few things:
I concur that we'd get Codeowners for free. (whew)
I agree we probably don't even need ApprovalProjectRules! If you made a default_project_rule_for and populated the project ID- that should be all we need? (Can't think of a downside off the top of my head)
I agree we probably don't even need ApprovalProjectRules! If you made a default_project_rule_for and populated the project ID- that should be all we need?
@garyh just want to clarify, do you mean that we don't need ApprovalRulesProject (not ApprovalProjectRules)? Just want to make sure I'm understanding this. If that's what you mean, yup, we don't need it. @kerrizor made me realize that in #254958 (comment 479633211) as well.
@patrickbajao@garyh I went ahead and updated the "Add relationships to new models" section with a little pseudocode sample to maybe help make that more clear.
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.
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.
FYI @danielgruesso as this may be of interest on your side as well. I'm actually not entirely sure where this would lie as this feels like implementation and not consumption.
@kerrizor I don't know if it's something groupcode review will explicitly prioritize, but it might be something we end up doing in support of other efforts. It's just not quite clear, yet, how we might line those things up.
@kerrizor My personal preference about refactoring things like this would be if there are features in our roadmap that will be dependent or will be easier to be worked on if we do this before we get to them. I think it relates with @phikai's comment above. If there's something approval rules related and we need to work on it, we can take a look if it'll benefit from this refactor.
A discussion with a customer (internal Slack thread) revealed a use case of "create a single approval rule that could be applied to all existing Protected Branches" which seems like a user action that a backend architecture like this would support.
For sure! We will probably start thinking about this for 16.11, but that depends on other work. Likely there will be some sync discussions, so we will be sure to invite you.
I've just had a long discussion with @ghinfey about implementing MVC: Allow group-level MR approval rules for 'A... (&11451), during that discussion we talked about different approaches that might fit in with the current architecture but are coming to the same realisations that @kerrizor has outlined in the description around performance, gaps between updating and syncing down the stack, and code complexity.
I think the outlined proposal makes a lot more sense and would deliver a much more polished UX with much greater flexibility. The current "fix" is actually quite bad, if you set it to "cannot edit rules" we do not copy any of the rules and instead use the project level rules at the MR level, however, if you do allow edit we still copy over the details for even rule even if unedited. This only half fixed the issue of "updating project level rules does not propagate down to MR level".
If this approach works well we may want to consider promoting this as a standard approach to this style of feature as I know we've been discussing moving project-level features to group-level recently. I suspect this will work for a lot of these features.