Closed mbauman closed 3 years ago
mbauman
commented
5 years ago Er, wait, maybe I'm misreading the log files. Here's the log I was looking at: https://build.julialang.org/#/builders/146/builds/473. I saw the git reset --hard fe6d412b9ca40785495479d6659fde326a15cf81 and saw that that was the last commit of the PR and attributed the failure to that... but now I see that the next line is checking out the PR's /merge branch. So I'm not sure why that failed now.
staticfloat
commented
5 years ago Yes, it appears that the github adapter chooses the tip of branch commit, rather than the merge commit. I believe this is because it's actually responding to the push event, not the PR itself. I don't know why the /merge stuff is in there, but the log very clearly shows git reset --hard <gitsha> even after the /merge stuff, so ¯\_(ツ)_/¯
KristofferC
commented
4 years ago Isn't this kinda bad? Like you get a green CI and you merge it and break master because it had other changes that were not reflected in the CI run.
staticfloat
commented
4 years ago @maleadt I think you were just talking to me about this on Slack.
So it looks to me like the issue is that, in the pull_request event we get from GitHub, buildbot extracts payload['pull_request']['head']['sha'] and payload['ref']. Seems reasonable enough, right? But that points to the head of the branch; to get the merge commit sha, we would need to pull payload['pull_request']['merge_commit_sha']. That should cause the revision and branch name to match up properly.
This shouldn't be too difficult to do; I'll work up a PR to try it out.
staticfloat
commented
4 years ago Hmmm, unfortunately it looks like my PR doesn't fix things quite right. There are a few problems:
It seems that the github "merge commits" are not immediately available upon webhook execution time. You have to wait a little bit before you can fetch them down, which is quite unfortunate.
The merge commits disappear, so re-running builds becomes nontrivial.
This breaks status reporting, as the statuses are no longer tied to the proper gitshas.
It's inconsistent; if a PR is not mergeable, there is no merge commit.
This makes me think perhaps what we should do is attempt a local merge instead. What do you guys think?
KristofferC
commented
4 years ago The merge commits disappear, so re-running builds becomes nontrivial.
You mean after the PR is merged or a new commit is added? It seems unlikely yo want to re-run such an old build.
This breaks status reporting, as the statuses are no longer tied to the proper gitshas.
You mean status to the GitHub PR? Can't that be fixed? Travis manages to report the status even though they run merge commits.
It's inconsistent; if a PR is not mergeable, there is no merge commit.
Why do you want to run CI on a non-mergable thing though? What would it tell you?
staticfloat
commented
4 years ago You mean after the PR is merged or a new commit is added?
After a new commit is added; so even if you have the gitsha of the previous merge commit on a PR, you can't re-run it. It also means that sometimes when you push two commits up one after another quickly, you get failed CI statuses because by the time the buildbots get around to building your (older) commits, the merge gitshas are gone.
Travis manages to report the status even though they run merge commits.
I think Travis' status reporting goes through a different method than the buildbot; when you look at Travis' results, it has a whole status page here on GitHub, so I'm not surprised that they can do something more complicated. We can probably solve this issue by modifying the buildbot GitHub status reporter to send a status for a revision that isn't actually the revision we tested.
Why do you want to run CI on a non-mergable thing though? What would it tell you?
That the changes you've made on your PR work as you expect? There are two questions you can ask with CI: the first is, "do my changes have the intended effect as they appear in my commit". The second is "do my changes interact poorly with all the changes made since I wrote it". Both are valuable, IMO.
KristofferC
commented
4 years ago even if you have the gitsha of the previous merge commit on a PR, you can't re-run it.
It seems unlikely that you want to re-run a commit on a PR that isn't the last one.
It also means that sometimes when you push two commits up one after another quickly, you get failed CI statuses because by the time the buildbots get around to building your (older) commits, the merge gitshas are gone.
The buildbot should (like Travis) probably skip testing the commit when there is a newer one in the PR. We had a custom script for that for AppVeyor (and Travis??) but Travis supports it directly now with an option.
That the changes you've made on your PR work as you expect?
I consider CI to answer the question if the build will still work after it is merged. With a merge conflict, that question cannot be answered. I don't remember this ever being an issue when using Travis.
We use CI to answer the question if it is safe to press the merge button. And in the current case, sometimes our CI doesn't even tell us about the state of the resulting build after pressing that button. Like we are doing a release, and then we have a final commit to put in. Someone restarts the PR and CI runs and all is green. But that commit could very well break the build if it was made a long time ago.
vchuravy
commented
4 years ago I consider CI to answer the question if the build will still work after it is merged.
To truly answer that we would need to use something like bors for merging, since I can merge two PRs back to back and one of them broke the other and both PRs will have green CI checks because the prior merge commit would have been ok.
KristofferC
commented
4 years ago To truly answer that we would need to use something like bors for merging, since I can merge two PRs back to back and one of them broke the other and both PRs will have green CI checks because the prior merge commit would have been ok.
Yes, I know :P. Baby steps.
DilumAluthge
commented
3 years ago We've now migrated analyzegc to Buildkite. I'm working on a PR that will configure Buildkite to use the merge commit.
Not sure if intentional or not, but I noticed this when re-running the tests on an old PR (that was so old it predated the analyzegc support). The buildbot checked out the last commit from the PR directly instead of grabbing the merge result.