servo / homu

A bot that integrates with GitHub and your favorite continuous integration service
MIT License
328 stars 51 forks source link

Approved PR gets built twice on auto branch #144

Open bstaletic opened 6 years ago

bstaletic commented 6 years ago

I'm part of the team that's maintaining YouCompleteMe and ycmd. We're using homu to merge our pull requests, but we've run into an annoying issue.

Once we approve a PR, it gets tested on the auto branch, then, provided, the tests passed, it is built and tested on master. So far so good. The next time a PR is approved, homu would first trigger another build of the previously merged PR on the auto branch.

Also, this doesn't only happen on successful merges, but even on PR's whose tests fail. To better show what I'm trying to explain, here's the travis history: https://travis-ci.org/Valloric/ycmd/builds/

If you open the link and search for #898 for example, you will see a build on auto, then master, then auto again.

I'm aware that this could be a misconfiguration on our end, so if it is I'd like to ask for help finding out what's wrong.

Manishearth commented 6 years ago

That seems weird. I'm looking at your settings and don't see anything off. Feels like a misconfiguration -- i don't see this pattern on other homu-using repos -- but I can't be sure.

Valloric commented 6 years ago

@Manishearth I can't see anything wrong with our homu config. Perhaps its a Travis config issue? I've included our homu config below (with secrets redacted).

[github]
access_token = "<redacted>"
app_client_id = "<redacted>"
app_client_secret = "<redacted>"

[web]
port = 54856
sync_on_start = true

[repo.ycmd]
owner = "Valloric"
name = "ycmd"

reviewers = []
auth_collaborators = true
try_users = []

[repo.ycmd.github]
secret = "<redacted>"

[repo.ycmd.status.travis]
context = 'continuous-integration/travis-ci/push'

[repo.ycmd.status.appveyor]
context = 'continuous-integration/appveyor/branch'

[repo.YouCompleteMe]
owner = "Valloric"
name = "YouCompleteMe"
reviewers = []
auth_collaborators = true
try_users = []

[repo.YouCompleteMe.github]
secret = "<redacted>"

[repo.YouCompleteMe.status.travis]
context = 'continuous-integration/travis-ci/push'

[repo.YouCompleteMe.status.appveyor]
context = 'continuous-integration/appveyor/branch'

[repo.EmacsYcmd]
owner = "abingham"
name = "emacs-ycmd"

reviewers = []
auth_collaborators = true
try_users = []

[repo.EmacsYcmd.github]
secret = "<redacted>"

[repo.EmacsYcmd.status.travis]
context = 'continuous-integration/travis-ci/push'

[repo.JediHTTP]
owner = "vheon"
name = "JediHTTP"

reviewers = []
auth_collaborators = true
try_users = []

[repo.JediHTTP.github]
secret = "<redacted>"

[repo.JediHTTP.status.travis]
context = 'continuous-integration/travis-ci/push'

[repo.JediHTTP.status.appveyor]
context = 'continuous-integration/appveyor/branch'

# The database homu uses
[db]

## SQLite file
file = "main.db"
Manishearth commented 6 years ago

Yeah i suspect a problem in your Travis config as well but I looked at the Travis stuff and it all seems fine. Unsure what's happening.

bstaletic commented 6 years ago

@Manishearth

@Valloric mentioned travis config, but that can't be the case because appveyor is doing the same thing, ie building twice on auto branch.

Valloric commented 6 years ago

@Manishearth I'm also chatting with @bstaletic in our Gitter room about this and he's pointed out that the same issue happens on Appveyor, which is a strong hint that it's probably not Travis config.

Valloric commented 6 years ago

It could be that we have messed up config for both Travis and Appveyor in exactly the same way, but that just seems plain unlikely.

bstaletic commented 6 years ago

I just looked at two other repos. vheon/JediHTTP and abingham/emacs-ycmd Both repos expreience the same issue of double building on auto branch.

ping @vheon @abingham

Valloric commented 6 years ago

@bstaletic Thanks for checking that! OK, so the odds of both @vheon and @abingham separately misconfiguring Travis CI in the same way that I did (along with me doing the same mistake on Appveyor) is pretty much zero.

So if it's not an issue with CI services or our homu config, the only thing that's left is a homu bug, right? Or am I crazy?

bstaletic commented 6 years ago

vheon/JediHTTP also uses Appveyor, which is as well experiencing this problem. So I strongly believe that this can't be a CI service misconfiguration.

Manishearth commented 6 years ago

Yeah, makes sense for it to be a homu bug. Don't have the bandwidth right now to investigate it but might be worth logging wherever homu explicitly requests a build.

bstaletic commented 6 years ago

@Manishearth Should we contact you somehow before triggering homu again? Or should we do the logging somehow?

Manishearth commented 6 years ago

Yeah I would add a bunch of printfs wherever homu makes decisions regarding triggering builds.

bstaletic commented 6 years ago

Okay, this PR should be merged without a long review, so keep an eye on it. https://github.com/Valloric/ycmd/pull/904

jdm commented 6 years ago

@Manishearth's comment was ambiguous, but I believe it should be read as "please add printfs wherever homu makes decisions regarding triggering builds".

bstaletic commented 6 years ago

Then I definitely understood it wrong.

Ping @Valloric

Valloric commented 6 years ago

(Copied from my comment on the linked PR)

Not sure it would be easy (or a good idea) to change our zzbot instance since it's running vanilla homu code. There's nothing custom to it. If more logging should be added to debug our issue, we should probably add this logging code to the real homu codebase (at DEBUG level, so off by default). Then others in the future could also benefit from this.

Hacking around our checkout of homu seems... iffy to me.