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

Jenkins CI status: Bot should not look for latest commit in PR #157

Open
addaleax opened this issue Nov 16, 2017 · 4 comments
Open

Jenkins CI status: Bot should not look for latest commit in PR #157

addaleax opened this issue Nov 16, 2017 · 4 comments

Comments

@addaleax
Copy link
Member

Right now the bot looks for the latest commit in a PR, but it should refer to the commit that the build was kicked off for.

It seems like there’s a commit field in the Jenkins push event body, so that should be pretty simple to do, I just don’t really know how to test this?

@phillipj
Copy link
Member

As far as I remember, we tried to base the Jenkins <-> GitHub PR status logic on the commit provided by Jenkins, but that SHA was unknown by GitHub. We therefore made the bot resolve the latest commit SHA of the PR branch instead.

For all I know, this has changed meaning the SHA that Jenkins provides is actually valid SHA on github.com?

@addaleax
Copy link
Member Author

Oh, I see. I assume that is/was because we do the rebasing before actually running test?

I don’t know, but if I look at things like https://ci.nodejs.org/job/node-test-commit/14072/ then the (commit: 683c07f) link is actually clickable and leads to commit from the Node PR… does that work?

@phillipj
Copy link
Member

We might give the commit SHA from Jenkins a new shot. What's provided to the bot by Jenkins post build has just gotten a significant refactor; https://github.com/nodejs/build/pull/973/files

Maybe @maclover7 has some input on this? Are there more than one commit SHA available that we can investigate is the one github.com knows?

@maclover7
Copy link
Contributor

Hmm, yeah, I believe the original reason is due to the rebasing done on certain platforms. If we want to try and change the settings during an off-hour-ish time, works for me, but an issue should probably be opened in nodejs/build too :)

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

No branches or pull requests

3 participants