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

can-merge incorrectly says the PR is mergeable #38

Open
ljharb opened this issue Dec 27, 2021 · 6 comments · Fixed by #40
Open

can-merge incorrectly says the PR is mergeable #38

ljharb opened this issue Dec 27, 2021 · 6 comments · Fixed by #40
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted PRs are welcome!

Comments

@ljharb
Copy link
Owner

ljharb commented Dec 27, 2021

On nvm-sh/nvm#2698, at a stage when every status check was green except for the Travis CI one, which was pending, I got this output:

$ can-merge --remote=source --pr=2698
PR status: 2698 ✔ This PR is mergeable
Pending Checks (1): Travis CI - Pull Request

It shouldn't report that the PR is mergeable while there's still a pending required check (since the travis check is required). Note that this output would have been perfect if travis was optional.

Note that the exit code was 1, which is correct.

@ljharb ljharb added bug Something isn't working help wanted PRs are welcome! good first issue Good for newcomers labels Dec 27, 2021
@ljharb ljharb changed the title can-merge incorrectly exits 0 can-merge incorrectly says the PR is mergeable Dec 27, 2021
@ljharb
Copy link
Owner Author

ljharb commented Dec 27, 2021

The final check just went green, and now I correctly get this output:

$ can-merge --remote=source --pr=2698
PR status: 2698 ✔ This PR is mergeable

with a zero exit code.

@ljharb
Copy link
Owner Author

ljharb commented Dec 31, 2021

For import-js/eslint-plugin-import#2341, I'm running can-merge --remote=source -p 2341 and getting:

PR: 2341 ✔ This PR is mergeable
Pending Checks (99): majors (16, 4), packages (15, 7, resolvers/webpack), majors (16, 3), packages (14, 8, resolvers/node), majors (16, 2), packages (14, 8, resolvers/webpack), majors (15, 7), packages (14, 7, resolvers/node), majors (15, 6), packages (14, 7, resolvers/webpack), majors (15, 5), packages (13, 8, resolvers/node), majors (15, 4), packages (13, 8, resolvers/webpack), majors (15, 3), packages (13, 7, resolvers/node), majors (15, 2), packages (13, 7, resolvers/webpack), majors (14, 8), packages (12, 8, resolvers/node), majors (14, 7), packages (12, 8, resolvers/webpack), majors (14, 6), packages (12, 7, resolvers/node), majors (14, 5), packages (12, 7, resolvers/webpack), majors (14, 4), packages (11, 8, resolvers/node), majors (14, 3), packages (11, 8, resolvers/webpack), majors (14, 2), packages (11, 7, resolvers/node), majors (13, 7), packages (11, 7, resolvers/webpack), majors (13, 6), packages (10, 8, resolvers/node), majors (13, 5), packages (10, 8, resolvers/webpack), majors (13, 4), packages (10, 7, resolvers/node), majors (13, 3), packages (10, 7, resolvers/webpack), majors (13, 2), packages (9, 8, resolvers/node), majors (12, 8), packages (9, 8, resolvers/webpack), majors (12, 7), packages (9, 7, resolvers/node), majors (12, 6), packages (9, 7, resolvers/webpack), majors (12, 5), packages (8, 8, resolvers/node), majors (12, 4), packages (8, 8, resolvers/webpack), majors (12, 3), packages (8, 7, resolvers/node), majors (12, 2), packages (8, 7, resolvers/webpack), majors (11, 7), packages (7, 8, resolvers/node), majors (11, 6), packages (7, 8, resolvers/webpack), majors (11, 5), packages (7, 7, resolvers/node), majors (11, 4), packages (7, 7, resolvers/webpack), majors (11, 3), packages (6, 8, resolvers/node), majors (11, 2), packages (6, 8, resolvers/webpack), majors (10, 7), packages (6, 7, resolvers/node), majors (10, 6), packages (6, 7, resolvers/webpack), majors (10, 5), majors (10, 4), majors (10, 3), majors (10, 2), majors (9, 6), majors (9, 5), majors (9, 4), majors (9, 3), majors (9, 2), majors (8, 6), majors (8, 5), majors (8, 4), majors (8, 3), majors (8, 2), majors (7, 5), majors (7, 4), majors (7, 3), majors (7, 2), majors (6, 5), majors (6, 4), majors (6, 3), majors (6, 2), majors (lts/*, 7, 3, 3), majors (lts/*, 7, 2, 2), Travis CI - Pull Request

with a zero exit code, which is incorrect, since there's required checks that aren't completed.

@ljharb
Copy link
Owner Author

ljharb commented Jan 6, 2022

This doesn't seem fixed yet.

@KhatiaIvanova
Copy link
Contributor

What exactly doesn't seem fixed?

@ljharb
Copy link
Owner Author

ljharb commented Mar 1, 2022

#40 did not fix the exit code behavior. I haven't checked against an actual PR since then, but I don't recall any code change which would have fixed it.

@KhatiaIvanova
Copy link
Contributor

Jordan I added code in #39 pull request. I just added one if check for conclusion pending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted PRs are welcome!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants