<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Work on this issue](https://contributors.gitlab.com/manage-issue?action=work&projectId=278964&issueIid=583995) </details> <!--IssueSummary end--> The following discussions from !214849 should be addressed: - [ ] @jnutt started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214849#note_2933062967): > **suggestion (non-blocking):** Is it better to use an instance variable here, or to define `attr_accessor :desired_default_branch` in the same file? This seems to achieve the same thing in a way that doesn't displease Rubocop, but it seems like a roundabout way of doing the same thing. > > It would make the tests a little easier to follow, since IMO `instance_variable_get`/`instance_variable_set` feel like a bit of an escape hatch. - [ ] @jnutt started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214849#note_2933062982): (+1 comment) > **question:** What do you mean by "required" in this context? That we were required to provide a reason, or that there's no effective way to do this without disabling the cop? I think we ought to expand on this. - [ ] @jnutt started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214849#note_2933063016): (+4 comments) > **question:** I was wondering if we could write these specs by actually updating the underlying repo, rather than stubbing, and I'm struggling. > > ```diff > diff --git a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb > index c94118af3e16..13aaf2d565de 100644 > --- a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb > +++ b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb > @@ -253,14 +253,13 @@ > > context 'when desired branch exists' do > before do > - allow(repository).to receive(:root_ref).and_return('main') > - allow(repository).to receive(:branch_exists?).with(new_branch).and_return(true) > + repository.create_branch(new_branch, repository.root_ref) > end > > it 'changes HEAD to the desired branch' do > - expect(repository).to receive(:change_head).with(new_branch) > - > - container.apply_desired_default_branch > + expect { container.apply_desired_default_branch } > + .to change { container.reload_default_branch } > + .from('master').to(new_branch) > end > > it 'reloads the default branch' do > > ``` > > This change causes the spec to fail, but I would have expected it to pass. Manually testing your change, it looks good, but this is making me worry that I don't understand the underlying mechanism well enough to approve this. Could you point me towards what I might be missing? :pray: - [ ] @SamWord started a [discussion](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214849#note_2940561094): > **Suggestion (non-blocking):** I think you could get away without using an instance variable at all here and just pass it to `apply_desired_default_branch` as an argument. `apply_desired_default_branch` is only called when setting `branch_name` on a model with `HasRespository`, and it's just whatever `branch_name` is anyway, so I don't think it'd result in any performance gains.