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

With outside PRs, githubstatus POSTs the wrong repo #512

Open
taktoa opened this issue Oct 10, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@taktoa
Copy link
Contributor

commented Oct 10, 2017

Suppose you set up a Hydra declarative jobset to build PRs using the githubpulls input on a GitHub repo named alice/foo. You then enable the Hydra githubstatus plugin with the intention of making it possible for contributors to see the build status easily. This requires you to give it a GitHub auth token with repo:status permission on an account that has push access to alice/foo. Now suppose a pull request comes in from bob/foo. Currently, the githubstatus will POST the commit status to alice/foo instead of bob/foo, and since alice/foo doesn't have that commit, it seems to fail.

Example: commit statuses are visible on local commits here, but are not visible on this pull request despite a jobset existing.

I suspect this issue was not noticed because many of the users of githubstatus are making feature branches on alice/foo and then doing a pull request against master, but this feature is pretty much essential to making Hydra usable for everyday open source projects on GitHub (as opposed to big industrial projects).

@domenkozar

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

@shlevy

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

The repo is determined by the relevant jobsetinput:

@shlevy

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

In fact, I'm now wondering if the problem isn't the exact opposite of what you suggest: You don't have permission to set status on bob, but if you instead tried setting status on alice it would work.

@taktoa

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

That sounds plausible; I haven't looked too deeply into the issue, so the explanation given in the initial post is nothing more than a hypothesis based on the observed symptoms.

@shlevy

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

My guess is adding something to the GithubStatus config to hard-code owner and repo would do the trick.

@domenkozar

This comment has been minimized.

Copy link
Member

commented Oct 21, 2017

Afaik most CIs merge the PR and then post the status on the merge commit build. Otherwise, you might not be able to attach the status on $rev since it's not part of the repo. Hopefully, I'm mistaken :)

@shlevy

This comment has been minimized.

Copy link
Member

commented Oct 21, 2017

@domenkozar They may merge locally, but I think you're mistaken about posting the status to the merged rev. The statuses are per rev and so itwouldn't show up on the unmerged rev in that case, plus then the CI would need push access to your repo!

I'm 90% sure this is due to posting on the source repo instead of the target repo. Note that I've successfully fetchgitted from a pull request before, so it seems the rev is already part of the target repo in some sense.

@cleverca22

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2017

https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref

by sending an http GET to the right url (no auth required), you can list all of the status messages for a given revision on a given project

@Gabriel439

This comment has been minimized.

Copy link

commented Feb 12, 2018

Okay, I figured out how to get Hydra's GithubStatus plugin working on pull requests originating from another repository.

The best way to explain this is to begin from an example of how NOT to do it, such as this line of code from the iohk-ops repository:

https://github.com/input-output-hk/iohk-ops/blob/7aa3a88dbbb68ea1bb9770cb2d26a831c52a623e/jobsets/default.nix#L72

 cardano = mkFetchGithub "https://github.com/${info.head.repo.owner.login}/${info.head.repo.name}.git ${info.head.ref}";

Most projects that enable Hydra's pull request support have a Nix expression which declaratively specifies how to build each pull request (like the above jobsets/default.nix) and that file will have some line similar to the the above line which says that the source input to the pull request jobset is a git input derived from the login/name/ref fields of the pull request metadata.

The above idiom is commonly cargo culted among Hydra projects setting up PR support, but is wrong in two separate ways:

First, you don't want to use the owner/name fields from the head of the pull request (i.e. the repository that the pull request originates from). You want to derive them from the base of the pull request (i.e. the repository that they are trying to merge against, a.k.a. your repository). The reason you want to set this to your repository is because that is the repository that you have permission to read/write GitHub statuses to. In other words, the first change is to replace info.head.repo with info.base.repo for the first two interpolated fields:

 cardano = mkFetchGithub "https://github.com/${info.base.repo.owner.login}/${info.base.repo.name}.git ${info.head.ref}";

Second, you don't want to use the ref to specify which revision to mark the status of, because ref corresponds to the branch name and not the SHA256 of the revision. That branch name is not valid within your own repository, so you need to change it to specify the revision for this to work. That means that the fix is to replace info.head.ref with info.head.sha, like this:

 cardano = mkFetchGithub "https://github.com/${info.base.repo.owner.login}/${info.base.repo.name}.git ${info.head.sha}";

Once you do that, Hydra's GitHub status plugin will work. Any revision that the pull request is trying to merge is valid within your own repository. If you have permission to read/write your own repository then you have permission to read/write the status of that revision (within the context of your repository).

@cleverca22

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

ive found its better to do ` "https://github.com/${info.base.repo.owner.login}/${info.base.repo.name}.git pull/${num}/head";
if you specify a sha, then the evaluator will never see new pushes to the PR, until the declarative jobset re-configures every pr on the system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.