Move generic code to gitlab-triage and keep only domain specific code here
The following discussion from !149 (merged) should be addressed:
-
@rymai started a discussion: (+2 comments) -
lib/extension.rb
sounds like a very generic name - Why not implementing the generic part of this feature into
gitlab-triage
directly (i.e.EngineExtension
,RulePolicyExtension
,IssueBuilderExtension#initialize
)? That way we'd only overrideIssueBuilderExtension#description
to add the heatmap.
-
-
Rename lib/extension.rb
-> !155 (merged) -
Introduce another object to contain the conditions and the raw resources -> https://gitlab.com/gitlab-org/gitlab-triage/issues/147 -
Expose the raw resources so we can extend it
The most offending code is:
yield(resources, conditions.merge(all_resources: all_resources))
@rymai had a comment about it: !149 (comment 182469796)
Could we change this method in
gitlab-triage
to directly yieldresources, conditions, resources_without_limit
instead? I find it hackish to mergeall_resources: all_resources
intoconditions
here...🤔
To be honest, I spent a lot of time figuring this out, and I got really confused around this code. This was because the object we put into BasePolicy#resources
can actually have different types for SummaryPolicy
and RulePolicy
.
I left a comment in lib/gitlab/triage/policies/summary_policy.rb
# Due to resources is a different type, this will never work
# FIXME: We should try to make sure type is consistent for resources
def comment?
false
end
This shares the same issue. Depending on the instance, resources
can mean different things. In RulePolicy
it's an array of resources, but in SummaryPolicy
it's a hash mapping from rules to the resources.
In order to fix the confusion here, I would like to introduce a new class wrapping it. I don't know how to name it yet, but it should carry the difference.
And then, we can attach this all_resources
(or raw resources) to this object, instead of attaching it to the conditions.
I need to attach it to the conditions for now, because I don't know how to properly attach it to either an array or a hash (!) If we try to detect that, it's going to be even more confusing...
I consider that a ~"technical debt" without adding an actual object for that part, and I think we need to fix it now before we move on, because we do need to add something on top of it now.