search

nodejs / build

Better build and test infra for Node.
503 stars 165 forks source link

CI comment ends up in wrong repo #2166

Open addaleax opened 4 years ago

addaleax commented 4 years ago

I kicked off CI for https://github.com/nodejs/quic/pull/157 (https://ci.nodejs.org/job/node-test-pull-request/25909/) but the bot commented the CI link on https://github.com/nodejs/node/issues/157.

phillipj commented 4 years ago

Looking at the related route that handles request from ci.nodejs.org regarding started jobs, it looks to me as if cURL command executed by ci.nodejs.org specifies node as the repo. Probably been hard coded since that job has historically only been used by that repo, right?

I currently don't have enough ci.nodejs.org privileges to see the bash script executed upon job start, but it should be pretty obvious at first glance if I remember correctly. The URL used in that script should be using $TARGET_REPO_NAME job parameter instead of a hard coded string of node.

Does that make sense? Maybe you've got privileges to give it a shot?

addaleax commented 4 years ago

I don’t think I have any special CI privileges.

phillipj commented 4 years ago

Hopefully someone in @nodejs/build with enough CI privileges can have a look?

richardlau commented 4 years ago

I don't have privileges either, but what's probably going on is that node-test-pull-request isn't passing on the TARGET_REPO_NAME parameter to the post-build-status-update job (which has a REPO parameter that defaults to node).

image There are three more instances in the Post-build Actions section. I did a quick sample and it doesn't look like any of the jobs started by node-test-pull-request (e.g. the linter) that call post-build-status-update pass on a REPO parameter so they all default to node too.

Trott commented 4 years ago

@nodejs/jenkins-admins

richardlau commented 4 years ago

The bot is being triggered in the CI by the following pipeline: https://github.com/nodejs/build/blob/master/jenkins/pipelines/post-build-status-update.jenkinsfile

It looks like my earlier hypothesis (https://github.com/nodejs/github-bot/issues/245#issuecomment-539711522) is correct -- we need to set the REPO parameter when we trigger https://ci.nodejs.org/job/post-build-status-update/ otherwise it defaults to node. That's quite a lot of places that need updating.

I'll transfer this issue over to build to look at after the security releases go out.

richardlau commented 4 years ago

I'll transfer this issue over to build to look at after the security releases go out.

Except I can't transfer (probably because I don't have the appropriate permissions for this repository).

phillipj commented 4 years ago

Ahh thanks for referencing that .jenkinsfile 👍 I haven't noticed those in the build repo before..

If you'd rather not update lots of Jenkins related stuff as you mention, let me know if you think this is better solved in github-bot somehow and want some help with that.

richardlau commented 4 years ago

I don't have privileges either,

I didn't at the time, I do now 😀. This does require updating all the node-test-commit-* + the linter job, preferably all at the same time. I'll see if I can carve out some time later next week.

mmarchini commented 4 years ago

Wouldn't updating the node-test-pull-request be enough at least to fix the comment? This one seems more urgent (I'm reluctant of testing auto start CI via labels because it will spam a bunch of old issues), updating the others could be done at a separate time.

richardlau commented 4 years ago

Wouldn't updating the node-test-pull-request be enough at least to fix the comment? This one seems more urgent (I'm reluctant of testing auto start CI via labels because it will spam a bunch of old issues), updating the others could be done at a separate time.

Updated just node-test-pull-request via https://ci.nodejs.org/job/node-test-pull-request/jobConfigHistory/showDiffFiles?timestamp1=2020-06-01_13-53-19&timestamp2=2020-06-27_15-37-02

richardlau commented 4 years ago

@mmarchini I started https://ci.nodejs.org/job/node-test-pull-request/32125/ but I don't think it has worked. It did trigger https://ci.nodejs.org/job/post-build-status-update/1028669/ but doesn't look like it commented in https://github.com/nodejs/node-auto-test/pull/19.

mmarchini commented 4 years ago

It's not commenting on node-auto-test, but it's also not commenting on nodejs/node. And when starting a CI on a PR from nodejs/node it still comments correctly. Not the best outcome, but I call it a win (since at least we're not spamming old issues) :)

We probably need more changes on github-bot? I don't have access to the machine so I can't look at the logs to know what's wrong.

mmarchini commented 4 years ago

Maybe the bot lacks permission to comment on other repos?

richardlau commented 4 years ago

Maybe the bot lacks permission to comment on other repos?

It's probably https://github.com/nodejs/github-bot/blob/4cce2b9b35f1d3db9d59c06f48d1d2dc63663260/scripts/jenkins-status.js#L4?

mmarchini commented 4 years ago

The bot doesn't have write permission on that repo. I made a request to add it: https://github.com/nodejs/node-auto-test/issues/20

phillipj commented 4 years ago

We probably need more changes on github-bot? I don't have access to the machine so I can't look at the logs to know what's wrong.

Protip; the logs are actually exposed over HTTPS. I'll send you a couple of curl examples on IRC..

github-actions[bot] commented 3 years ago

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.