Follow-up from "Merge trains POST API"
Hey @markus.ferrell, welcome to the follow-up issue. All the notes I made on !100853 (merged) are clarity-related nitpicks, not blockers. So I've resolved them there, setting up that (lengthy!) MR to be merged and moved the notes to this follow-up issue that we can address with another MR. I'd be happy to do the review on any MRs for this.
The following discussions from !100853 (merged) should be addressed:
-
@drew started a discussion: This test case seems to be testing the same thing as above:
context 'with invalid parameters' do let(:params) { { sha: 123, squash: 'yes', when_pipeline_succeeds: 'yes' } } it 'errors out before calling service' do subject expect(response).to have_gitlab_http_status(:conflict) end end
Can we get rid of one? Or do you want to use a correct
sha
in the above context to test errors from the badsquash
andwhen_pipeline_succeeds
values? -
@drew started a discussion: context 'when pipeline is not completed' do
😁 -
@drew started a discussion: There is an extremely small chance of this happening, but since
12345
is technically a validiid
there's a conceptual chance this spec could fail due to spec ordering. It's a super easy fix to...-1
or something so we don't ever have to think about it😅 -
@drew started a discussion: (+1 comment) optional :when_pipeline_succeeds, type: Grape::API::Boolean,
-
@drew also noticed we're missing a couple response codes in the endpoint desc
, at leastConflict
andForbidden
. Do a rundown of the specs and services to make sure we haven't left any others out?desc 'Add a merge request to a merge train' do detail 'This feature was introduced in GitLab 15.6' success [ { code: 201, model: EE::API::Entities::MergeTrain }, { code: 202, model: EE::API::Entities::MergeTrain } ] failure [ { code: 400, message: 'Failed to merge' }, { code: 401, message: 'Unauthorized' }, { code: 404, message: 'Not found' } ] end
context 'when sha is provided and doesn\'t match' do let(:params) { { sha: 'SomeFakeSha' } } it 'returns status conflict' do subject expect(response).to have_gitlab_http_status(:conflict) end end context 'when user is guest' do let(:user) { guest } it 'returns forbidden before reaching the api endpoint' do subject expect(response).to have_gitlab_http_status(:forbidden) end end