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] Infer PR for watch #43

Merged
merged 3 commits into from
Jan 10, 2022
Merged

[Fix] Infer PR for watch #43

merged 3 commits into from
Jan 10, 2022

Conversation

Green-Ranger11
Copy link
Collaborator

Removes check for whether PR is specified so that watch can infer PR. Also adds a check so that if sha and pr are not able to be inferred or specified an error is thrown.

This closes #42

bin/can-merge Outdated
Comment on lines 101 to 103
if (argv.watch && !argv.sha && !argv.pr) {
throw chalk.red('Could not infer sha. Please specify a pull request to watch with --pr');
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (argv.watch && !argv.sha && !argv.pr) {
throw chalk.red('Could not infer sha. Please specify a pull request to watch with --pr');
}
if (argv.watch && !argv.sha && !argv.pr) {
throw chalk.red('Could not infer a PR or a SHA. Please specify a pull request to watch with `--pr`, or a commit with `--sha`');
}

@ljharb
Copy link
Owner

ljharb commented Jan 8, 2022

Using this PR, i ran this:

can-merge --remote=source --watch && git push source

on ecma262, on tc39/ecma262#2605. I got a bunch of dots, and then expected it to be completed and push up the changes. However, I got:

PR: 2605 ✔ This PR is mergeable
..........Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 16 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 1.59 KiB | 1.59 MiB/s, done.
Total 3 (delta 2), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/main.
remote: error: Required status check "Begin.com build preview" is expected.
To github.com:tc39/ecma262.git
 ! [remote rejected]   HEAD -> main (protected branch hook declined)
error: failed to push some refs to 'github.com:tc39/ecma262.git'

So it seems like it was missing this required status - possibly because the status is not actually set on the PR until later in the process?

@Green-Ranger11
Copy link
Collaborator Author

Hmmm I am confused though by what you mean by "not set on the PR until later in process"?

@ljharb
Copy link
Owner

ljharb commented Jan 8, 2022

What i mean is, most statuses get set on the PR as “pending” right away. In this case, the deploy preview status is not set at all until after the build status is completed. However, because the repo has it as “required”, it shows up anyways on the PR, but may not show up in the api response.

@Green-Ranger11
Copy link
Collaborator Author

It might be we're not correctly querying GitHub as well in addition to the point you mentioned. I will do some research and get back in addition to fixing the tests.

@ljharb
Copy link
Owner

ljharb commented Jan 9, 2022

I agree it's not specifically related to this PR, since it happens with and without --watch, but I wanted to call it out.

@ljharb
Copy link
Owner

ljharb commented Jan 9, 2022

(This one can land once tests are passing)

@ljharb ljharb merged commit 6785fc6 into main Jan 10, 2022
@ljharb ljharb deleted the fix/infer-watch-pr branch January 10, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--watch does not recognize inferred PR(s)
2 participants