Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gitlab): verify if MR is mergeable before setting automerge #6250

Merged
merged 5 commits into from May 28, 2020

Conversation

tmeijn
Copy link
Contributor

@tmeijn tmeijn commented May 15, 2020

This polls the Gitlab Merge requests API for the required status that's needed before setting merge_when_pipeline_succeeds to true.

Closes #6248

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to mock delay in tests: jest.mock('delay', () => Promise.resolve());

@CLAassistant
Copy link

CLAassistant commented May 15, 2020

CLA assistant check
All committers have signed the CLA.

@tmeijn
Copy link
Contributor Author

tmeijn commented May 15, 2020

@viceice Sorry I have little to no experience with testing. I tried to do what you suggested, but it still seems to fail. Can you help me out?

@viceice
Copy link
Member

viceice commented May 15, 2020

ok, can you first fix your commits to use the right author

@viceice
Copy link
Member

viceice commented May 15, 2020

You need to add the following after describe('createPr(branchName, title, body)', () => {.

line ~956

    beforeEach(() => {
      api.get.mockResolvedValueOnce(
        partial<GotResponse>({
          body: {
            merge_status: 'can_be_merged',
            pipeline: {
              id: 29626725,
              sha: '2be7ddb704c7b6b83732fdd5b9f09d5a397b5f8f',
              ref: 'patch-28',
              status: 'success',
            },
          },
        })
      );
    });

This polls the Gitlab Merge requests API for the required status that's needed before setting `merge_when_pipeline_succeeds` to `true`.

Closes renovatebot#6248
@tmeijn
Copy link
Contributor Author

tmeijn commented May 16, 2020

@viceice @rarkins Any pointers on how to proceed? It fails on code coverage, saying the await delay statement is not covered. Again, no experience with testing.

@tmeijn tmeijn requested a review from viceice May 25, 2020 11:24
lib/platform/gitlab/index.spec.ts Outdated Show resolved Hide resolved
lib/platform/gitlab/index.spec.ts Outdated Show resolved Hide resolved
@viceice viceice marked this pull request as ready for review May 28, 2020 05:53
@viceice viceice requested a review from rarkins May 28, 2020 05:53
@rarkins
Copy link
Collaborator

rarkins commented May 28, 2020

Why is this change necessary? It will slow down automerging for GitLab users unless it succeeds in setting the flag in the same run the MR is created in.

Edit: Sorry, now I see the linked issue. Seems it's workaround for a GitLab flaw. Do you expect that in most cases the flag will still be set in the first run, or in a later run? Have you verified that the logic will still work on a later run?

@tmeijn
Copy link
Contributor Author

tmeijn commented May 28, 2020

Do you expect that in most cases the flag will still be set in the first run, or in a later run? Have you verified that the logic will still work on a later run?

So I'm on Renovate 19.76.3 and of these MRs:

image

(Log of the relevant job can by found here: https://pastebin.com/YHaZXhW7)

all of them except protractor should've been Gitlab automerged by setting merge when pipeline succeeds. They were not however, they were merged by a second run of Renovate bot. You can see in the log they all returned 405 when trying to set the flag.

If you mean a later run of Renovate bot, I do not think Renovate as of 19.76.3 tries to set the merge when pipeline succeeds flag when a merge request is already created in a previous run.

Difficult thing is, I tried to reason about this as thorough as I could by going through the docs and issues in Gitlab, but I didn't hear anybody else here having this issue.

@rarkins rarkins merged commit fa3e017 into renovatebot:master May 28, 2020
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 20.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitlab: "405 method not allowed" when enabling gitlabAutomerge
5 participants