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

Get Pull Request ID by branch name or commit hash #190

Merged
merged 1 commit into from Sep 14, 2019
Merged

Get Pull Request ID by branch name or commit hash #190

merged 1 commit into from Sep 14, 2019

Conversation

mgrachev
Copy link
Member

@mgrachev mgrachev commented Nov 6, 2018

Hi there!

Some CI-services don't provide information such as Pull Request ID. So, I decided to get Pull Request ID by branch name or commit hash.

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Nov 18, 2018

@haya14busa What do you think about it?

@haya14busa
Copy link
Member

@haya14busa haya14busa commented Mar 20, 2019

I'm sorry for the late response. I think it might be a good idea as a fallback. Do you have some CI examples which don't support p-r number?

[optional]
@shogo82148 Can you review this PR?

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Mar 20, 2019

I know at least the one CI service, which doesn't provide Pull Request ID - Vexor.

I have been using my patched version of reviewdog on Vexor since the previous November.
After that, I didn't have any problems with reviewdog on Vexor 😃

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Mar 25, 2019

I've fixed all conflicts.

Copy link
Member

@haya14busa haya14busa left a comment

This is breaking changes and I'm wondering what should we do.
e.g. Travis CI runs 2 builds, commit and p-r build.
Prior to this change, reviewdog only works on p-r build.

@reviewdog/reviewdog Do you have any thought?

Maybe we can document this change and the user can suppress reviewdog on commit build?
Worst case, 2 reviewdog runs in parallel and post duplicated comments.

cmd/reviewdog/main.go Outdated Show resolved Hide resolved
cmd/reviewdog/main.go Outdated Show resolved Hide resolved
cmd/reviewdog/main.go Outdated Show resolved Hide resolved
cmd/reviewdog/main.go Outdated Show resolved Hide resolved
@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Mar 26, 2019

Done.

cmd/reviewdog/main.go Outdated Show resolved Hide resolved
cmd/reviewdog/main.go Show resolved Hide resolved
@shogo82148
Copy link
Contributor

@shogo82148 shogo82148 commented Mar 28, 2019

I think it is the CI's issue. You should send a feature request to vexor.io first.

As haya14busa say, this feature is a breaking change. So it should be opt-in feature, and enabled by an environment value (e.g. REVIEWDOG_GUESS_PULL_REQUEST) or reviewdog config file.

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Apr 1, 2019

@haya14busa
Copy link
Member

@haya14busa haya14busa commented Apr 12, 2019

Sry for the late reply. I should have left comments earlier.

I don't like the idea to change the behavior by an environment variable. Environment variables should be used for passing API key, CI "environment" variable (like commit, pr-number, etc... in CI services) and configuration which doesn't change the behavior but changing target or something (e.g. GITHUB_API base endpoint).
(Actually, the last one might be good to set by command-line flag or some configuration file, but I decided to use env var before.)

Let's use the command-line flag instead. Also, as mattn said, reviewdog should exit with non-zero exit code if the user explicitly specifies the flag and couldn't find the p-r.

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Apr 15, 2019

@haya14busa @mattn Updated with using the flag guess.

About exiting with a non-zero code. I don't think that it's a good idea because CI-services create not only PR-builds. Thus, tests will always fail for these builds.

I think reviewdog shouldn't do anything if it doesn't find Pull Request ID by the branch name and the commit SHA. And it should exit with the zero code.

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Apr 22, 2019

@haya14busa What do you think about it? Do you have any comments?

@mgrachev
Copy link
Member Author

@mgrachev mgrachev commented Jul 17, 2019

@haya14busa Is there a chance to merge this PR? 🙏

Copy link
Member

@haya14busa haya14busa left a comment

I'm sorry for the late response.

I'm still not 100% sure that it's a good approach, but it should be useful for some cases and acceptable design.

Thank you for your contribution!

@haya14busa haya14busa merged commit 4361d2a into reviewdog:master Sep 14, 2019
16 checks passed
@mgrachev mgrachev deleted the feature/get-pull-request-id branch Sep 16, 2019
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.

None yet

4 participants