node-chakracore icon indicating copy to clipboard operation
node-chakracore copied to clipboard

Published 20 hours ago • verified publisher icon nodejs

CI: enable CI for node-chakracore PRs?

Open obastemur opened this issue 7 years ago • 9 comments

It could be nice to see what my PR is actually doing on supported systems. Especially, If there is a fail on a system that I don't currently have, it could be nice to see the logs of the fail (if any).

obastemur avatar Nov 14 '17 19:11 obastemur

@joaocgreis any thoughts on whether we can have GitHub CI setup for the node-chakracore repo? While we could manually trigger CI, curious if there's any reason why we can't have a web hook set up for this?

digitalinfinity avatar Nov 14 '17 19:11 digitalinfinity

@obastemur you are a member of the node-chakracore team, which means you have run permission in https://ci.nodejs.org/view/All/job/chakracore-test-pull-request/ . Just go there, click "Build with parameters", enter the PR number and there you go. The "Certify Safe" checkbox is a reminder that the jobs do not run on sandboxes, so when running the job for code that comes from outside contributors please take a look to make sure it's not trying to run anything harmful.

@digitalinfinity about automating the whole process: in node we don't want the CI to start automatically mainly for 2 reasons: the safety issue above and the PRs frequently need adjustments before it makes sense to start CI.

joaocgreis avatar Nov 14 '17 23:11 joaocgreis

@joaocgreis how an external contributor is able to see the fail logs? (if PR is failing on a platform)

EDIT: In other words, is there a way to do that ? previously, I wasn't able to see fail logs.

obastemur avatar Nov 14 '17 23:11 obastemur

@obastemur the "Console output" is public on all jobs (logs are only kept for a few days though).

I started a run on master: https://ci.nodejs.org/job/chakracore-test/193/ and it is not going very well.. Windows failed because a node-gyp issue that is not obvious to me (I did a run last week and Windows was good: https://ci.nodejs.org/job/chakracore-test/192/). I expect Linux and OSX to fail because of processes left behind (but the console output last time showed that all tests that failed were flaky). The linter is also failing, not obvious why.

joaocgreis avatar Nov 15 '17 00:11 joaocgreis

The windows test failure was a bug in the shim that was fixed a short time ago.

MSLaguana avatar Nov 15 '17 00:11 MSLaguana

I see the potential issues with not enabling it by default.

However, it could be nice to get a Good to merge approval from a CI before merge. Reviewer could simply tell to CI bot to do the job.. i.e.

  • Review is done, CI test this please!

Step by step..

  • A new PR comes in.. CI receives the webhook and adds a UI box with PR is not safe to merge.. (right on top of Merge button)
  • Reviewer reviews the PR.. final checks etc..
  • Reviewer comments Review is done, CI test this please .. CI checks if reviewer is the member of the team and triggers test run.
  • CI updates the message with the result from the test run.
  • If CI message was OKAY, reviewer may merge the PR.

obastemur avatar Nov 15 '17 00:11 obastemur

I'm positive there was some discussion around starting CI from GH comments, but I can't find it. The idea sounds good to me, it's possible that just no one had the time to move ahead with it yet.

cc @nodejs/github-bot

joaocgreis avatar Nov 23 '17 00:11 joaocgreis

Yupp, the @nodejs-github-bot has functionality for triggering CI from github.com comments: https://github.com/nodejs/github-bot/pull/128

Sadly it has stopped working due to some Jenkins permission issues: https://github.com/nodejs/github-bot/issues/146. Might be trivial to sort out if anyone's interested in digging into the issue.

phillipj avatar Nov 23 '17 13:11 phillipj

@phillipj I am interested in helping this one (as well as for other repos, mainly node core). I'll be a bit busy until Dec though

joyeecheung avatar Nov 24 '17 02:11 joyeecheung