The following discussions from !136232 should be addressed: - [-] @project_278964_bot_e50c5a74c24642c571054e7c8aedefc4 started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136232#note_1646331138 "Do not require MRs to be rebased when using FF Merge trains"): > ```suggestion > let_it_be(:project) { create(:project, :repository) } > ``` > > Project creations are very slow. To improve test performance, consider using `let_it_be`, `build`, or `build_stubbed` instead. > > :warning:️ **Warning**: If your test modifies data, `let_it_be` may be unsuitable, and cause state leaks! Use `let_it_be_with_reload` or `let_it_be_with_refind` instead. > > Unsure which method to use? See the [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#optimize-factory-usage) for background information and alternative options for optimizing factory usage. > > If you're concerned about causing state leaks, or if you know `let` or `let!` are the better options, ignore this comment. > > ([Improve this message?](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/specs/project_factory_suggestion.rb)) This is a won't do because it breaks the spec. - [ ] @hfyngvason started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136232#note_1660972952 "Do not require MRs to be rebased when using FF Merge trains"): > There are not a lot of [tests for `#should_be_rebased?` in core](https://gitlab.com/gitlab-org/gitlab/-/blob/4881a054e7928e9e4233a0e50ab2476fc1cc1820/spec/models/merge_request_spec.rb#L5041-5047). 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](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136232#note_1660972969 "Do not require MRs to be rebased when using FF Merge trains"): > All of these are project settings. Would it make sense to instead define a project method? Something like: > > ```ruby > # 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](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136232#note_1660972983 "Do not require MRs to be rebased when using FF Merge trains"): > Hmm, are there cases where `active?` is `true` but no pipeline is present? In that case, we could have `pipeline&.sha` both and `.dig(...)` be `nil`, and this method would incorrectly return `true`. (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: > > ```suggestion:-0+0 > 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?