Follow-up from "Do not require MRs to be rebased when using FF Merge trains"
The following discussions from !136232 (merged) should be addressed:
-
[-] @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a discussion:
let_it_be(:project) { create(:project, :repository) }
Project creations are very slow. To improve test performance, consider using
let_it_be
,build
, orbuild_stubbed
instead.⚠ ️ Warning: If your test modifies data,let_it_be
may be unsuitable, and cause state leaks! Uselet_it_be_with_reload
orlet_it_be_with_refind
instead.Unsure which method to use? See the testing best practices for background information and alternative options for optimizing factory usage.
If you're concerned about causing state leaks, or if you know
let
orlet!
are the better options, ignore this comment.
This is a won't do because it breaks the spec.
-
@hfyngvason started a discussion: There are not a lot of tests for
#should_be_rebased?
in core. Maybe we should move this to core instead of removing it?Looks like there are also no tests for the
#ff_merge_possible?
method. Since we noticed, how about we add those or create a follow-up? -
@hfyngvason started a discussion: All of these are project settings. Would it make sense to instead define a project method? Something like:
# ee/app/models/ee/project.rb def auto_rebasing_merge_trains_enabled? ::Feature.enabled?(:fast_forward_merge_trains_support, self) && merge_trains_enabled? && ff_merge_must_be_possible? end
But if you have a strong preference for keeping it here, I'm also fine with that.
-
@hfyngvason started a discussion: Hmm, are there cases where
active?
istrue
but no pipeline is present? In that case, we could havepipeline&.sha
both and.dig(...)
benil
, and this method would incorrectly returntrue
. (It's not a very consequential bug because either FF merges are not enabled and hence no rebase required, or the MR would soon be added to an FF train anyway).It seems that way, e.g. just after the car is created, before we call
refresh_pipeline!
. So maybe we should do:commit_sha = merge_request.merge_params.dig('train_ref', 'commit_sha') return false unless commit_sha.present? active? && pipeline&.sha == commit_sha
This bug was already present in the old code, but might be good to fix now? What do you think?