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

CI comment ends up in wrong repo #2166

Open
addaleax opened this issue Oct 8, 2019 · 18 comments
Open

CI comment ends up in wrong repo #2166

addaleax opened this issue Oct 8, 2019 · 18 comments

Comments

@addaleax
Copy link
Member

addaleax commented Oct 8, 2019

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

@phillipj
Copy link
Member

phillipj commented Oct 8, 2019

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
Copy link
Member Author

addaleax commented Oct 8, 2019

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

@phillipj
Copy link
Member

phillipj commented Oct 8, 2019

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

@richardlau
Copy link
Member

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
Copy link
Member

Trott commented Oct 9, 2019

@nodejs/jenkins-admins

@richardlau
Copy link
Member

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
Copy link
Member

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
Copy link
Member

phillipj commented Feb 5, 2020

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
Copy link
Member

richardlau commented Jun 27, 2020

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

richardlau commented Jun 27, 2020

@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 nodejs/node-auto-test#19.

@mmarchini
Copy link
Contributor

mmarchini commented Jun 27, 2020

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
Copy link
Contributor

Maybe the bot lacks permission to comment on other repos?

@richardlau
Copy link
Member

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
Copy link
Contributor

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

@phillipj
Copy link
Member

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants