-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Lint PR merge commit #61
Conversation
5ffbe33
to
ccbaba3
Compare
ref_merge = repo.refs['pull/{pr}/merge'.format(pr=pr_id)] | ||
except GitCommandError: | ||
# Either `merge` doesn't exist because the PR was opened | ||
# in conflict or it is closed and it can't be the latter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes the merge
ref isn't defined.
First case is when the PR is closed. This case should be handled already by this line (only running on open PRs).
Second case is when the PR has always had merge conflicts since it was opened. If the conflicts were ever resolved in the PR, then this is no longer a concern. However if merge conflicts enter the PR, merge
ref is still defined (though incorrectly). This is a case that we do have to handle.
Currently we don't do anything about these here, and instead check on this line for merge conflicts. This seems to work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR Sometimes there is no merge ref basically must be a merge conflict. This is already caught with PR ( #55 ). So we don't need to do anything here.
ccbaba3
to
048a003
Compare
Also can you please take a look at this one when you have a chance, @pelson? |
cffecfb
to
b8750a9
Compare
b8750a9
to
42605d8
Compare
Merging so as to clear out a piece of PR ( #53 ) and make that one clearer. |
Switches to linting the merge commit of PRs instead of the head commit. This is inline with what many other CIs are doing (like Travis CI and AppVeyor). The main reason to do this is to ensure we are not getting false positives from the linter. For example, imagine our PR has an error due to some part of the recipe that is unchanged, but the base branch contains a correction to this issue. We don't want this linting error as it is a false positive and will go away with a simple merge into the base branch.
It is possible to do this now as we correctly raise merge conflicts as an error thanks to PR ( #55 ). The functionality presented here is a smaller piece of the functionality already in PR ( #53 ) to optionally ignore recipes from the base branch. A test is provided to ensure we are getting the intended behavior by using PR ( #54 ).