Skip to content

Post CI comment in PR for node-test-pull-request builds #224

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.

Sign up for GitHub

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

Jump to bottom
Merged
phillipj merged 3 commits into from
Mar 25, 2019
Merged

Post CI comment in PR for node-test-pull-request builds #224

phillipj merged 3 commits into from
Mar 25, 2019

Conversation

@phillipj
Copy link
Member

@phillipj phillipj commented Mar 23, 2019

These changes posts a CI style comment to PR's whenever Jenkins reports that node-test-pull-request builds has started.

Closes #212

/cc @nodejs/github-bot

P.S. temporary non-master base branch as these changes builds on #222.

refack
refack previously requested changes Mar 23, 2019
View reviewed changes
@refack refack dismissed their stale review March 24, 2019 13:11

Ok if only part of "starting" flow

@phillipj phillipj changed the base branch from fix-push-jenkins-tests to master March 24, 2019 20:25
phillipj added 3 commits March 24, 2019 21:41
@phillipj
jenkins: post CI comment in PR for node-test-pull-request builds
fd57ce8 
These changes posts CI style comments whenever Jenkins reports that
node-test-pull-request builds has started.

Refs nodejs#212
@phillipj
jenkins: ensure job status === pending before posting CI comment
f4d73f5
@phillipj
jenkins: move HTTP assertions from .tearDown() alongside response test
6f53aea 
As it's way more intuitive to have those outgoing HTTP request
assertions alongside other assertions, than as part of the test tear
down process.
@phillipj phillipj force-pushed the create-ci-comment-in-prs branch from 2a95d13 to 6f53aea Compare March 24, 2019 20:43
@phillipj
Copy link
Member Author

phillipj commented Mar 24, 2019

Rebased, force pushed and fixed pending status check after #222 got merged (which caused some merge conflicts).

@phillipj phillipj merged commit 4737a16 into nodejs:master Mar 25, 2019
@phillipj phillipj deleted the create-ci-comment-in-prs branch March 25, 2019 12:52
@refack
Copy link
Contributor

refack commented Mar 25, 2019

image
🎉

@phillipj
Copy link
Member Author

phillipj commented Mar 25, 2019

Yeay very cool!

@refack
Copy link
Contributor

refack commented Mar 25, 2019

image

So it posts for "Resume" as well...
We might need an opt-out mechanism (less harsh them a GitHub "block user")

@phillipj
Copy link
Member Author

phillipj commented Mar 25, 2019

Could you elaborate what you'd rather see in the example you posted above? I'm not entirely sure what you mean by "resume", but since all those URLs are different, is it wrong to treat them like different builds all together?

First thought that came to mind to opt-out is allowing collaborators to add a special kind of label to the PR, e.g. bot-no-ci-comment or similar 🤷‍♂️

@refack
Copy link
Contributor

refack commented Mar 25, 2019

Just to be clear, I'd wait until someone actually complains.

what you'd rather see

I'm perfectly happy with the status quo, but others might find this inconvenient...
Posted it here more as a way to document the behavior.

First thought that came to mind to opt-out is allowing collaborators to add a special kind of label to the PR, e.g. bot-no-ci-comment or similar 🤷‍♂️

Sound great.

what you mean by "resume"

For CI jobs with flakes we use:
image
which runs only the failed parts.

@phillipj phillipj mentioned this pull request Mar 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@phillipj @refack