doc: add descriptions of CI jobs #12555

morrme · 2017-04-20T22:33:59Z

Fixes: #12021

Checklist

[x ] documentation is changed or added
commit message follows commit guidelines

gibfahn

Mostly looks good! Couple of minor nits.

Could you please wrap to 80 chars? Obviously don't break up a line/link if it messes up the Markdown formatting.

gibfahn · 2017-04-21T08:46:57Z

COLLABORATOR_GUIDE.md

+only runs the linter targets, which is useful for changes that only affect comments or documentation.
+
+* [`citgm-smoker`](https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/)
+uses CitGM to allow you to run npm install && npm test on a large selection of common modules. This is useful to check whether a change will cause breakage in the ecosystem. To test node ABI changes you can run [citgm-abi-smoker](https://ci.nodejs.org/view/Node.js-citgm/job/citgm-abi-smoker/).


Maybe add a link to CitGM:

CitGM -> [CitGM](https://github.com/nodejs/citgm)

Backticks:

npm install && npm test -> npm install && npm test

For consistency:

[citgm-abi-smoker] -> [citgm-abi-smoker]

gibfahn · 2017-04-21T10:21:50Z

COLLABORATOR_GUIDE.md

@@ -87,6 +87,23 @@ All pull requests that modify executable code should be subjected to
 continuous integration tests on the
 [project CI server](https://ci.nodejs.org/).

+Here is a summary of current CI jobs:


Maybe:

Here are some useful CI jobs:

We don't want to imply that this is an exhaustive list.

gibfahn · 2017-04-21T10:28:04Z

Please could you format your commit message according to the Contributing Guidelines?

Maybe something like this?

doc: update COLLABORATOR_GUIDE.md

Fixes: https://github.com/nodejs/node/issues/12021

If you don't have time it can be rewritten by whoever lands this, but if you are going to help out more on the project (as I hope you do), then it saves work if you produce better commit messages. The Fixes: line means that the issue will automatically be closed when this PR lands.

Also at the moment your Git author name and email address are set to:

morrme <morrme@users.noreply.github.com>

People usually choose to use their full names for commits. To set your name globally you can do:

git config --global user.name "REPLACE ME"
git config --global user.email "memthings@gmail.com" # Or whichever email address you prefer.

To change the author for a single commit you can do:

git commit --amend --author="REPLACE ME <memthings@gmail.com>"
git push --force-with-lease

If you don't want to have your name against this commit, that's fine too.

Fixes: #12021

gibfahn · 2017-04-21T12:44:39Z

@morrme thanks a lot for doing this! Suggesting changes is no trouble when people are responsive.

Let me know if you want me to change your name/email on landing.

morrme · 2017-04-21T12:55:10Z

@gibfahn it was my pleasure !

as for the name: no, it's no big deal, as long as it doesn't affect anything on your end.

mhdawson

LGTM

richardlau · 2017-04-21T17:59:16Z

COLLABORATOR_GUIDE.md

+only runs the linter targets, which is useful for changes that only affect comments 
+or documentation.
+
+* [`citgm-smoker`](https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/)


This could be the shorter https://ci.nodejs.org/job/citgm-smoker/ if you omit the view/Node.js-citgm. Same for citgm-abi-smoker later in this paragraph.

richardlau · 2017-04-21T18:02:29Z

COLLABORATOR_GUIDE.md

+is designed to allow one to run a single test over and over on a specific platform
+to confirm that the test is reliable.
+
+* [`node-test-commit`](https://ci.nodejs.org/job/node-test-commit/)


Is this job intended to be manually run, as opposed to being started by node-test-pull-request? If there isn't a good use case for running this job without running node-test-pull-request then I would be inclined not to include it as a separate bullet point.

It is sometime manually run when people just want to test a commit rather than a Pull Request. I don't think there's any particular reason to, as you can just choose not to rebase, which is all node-test-pull-request really does before starting node-test-commit. So it probably does make sense not to include it for now. If someone has a good reason to re-add it, they can do it later.

Fixes: #12021

Fixes: #12555

jasnell · 2017-04-21T23:33:48Z

COLLABORATOR_GUIDE.md

@@ -318,7 +337,7 @@ information regarding the change process:

 - A `PR-URL:` line that references the *full* GitHub URL of the original
  pull request being merged so it's easy to trace a commit back to the
-  conversation that led up to that change.
+  conversation that led up to that change.   


Extraneous whitespace change here?

jasnell · 2017-04-21T23:34:13Z

COLLABORATOR_GUIDE.md

@@ -87,6 +87,25 @@ All pull requests that modify executable code should be subjected to
 continuous integration tests on the
 [project CI server](https://ci.nodejs.org/).

+Here are some useful CI jobs:


Perhaps make this a heading?

#### Useful CI Jobs

Should I make the change now or wait until someone else agrees ? Not sure of the protocol here. @gibfahn ?

@morrme if it seems reasonable to you then go for it. The only downside to implementing suggested changes is that if lots of people disagree you might have to change it back. This one doesn't seem very controversial, so yeah I'd go ahead and change it.

lpinca

LGTM with @jasnell's comments addressed.

Fixes: #12021

morrme · 2017-04-22T19:57:33Z

COLLABORATOR_GUIDE.md

+* [`citgm-smoker`](https://ci.nodejs.org/job/citgm-smoker/)
+uses [`CitGM`](https://github.com/nodejs/citgm) to allow you to run `npm install && npm test` 
+on a large selection of common modules. This is useful to check whether a change will 
+cause breakage in the ecosystem. To test node ABI changes you can run [`citgm-abi-smoker`](https://ci.nodejs.org/job/citgm-abi-smoker/).


Should this be changed to

To test Node ABI changes

or

To test Node.js ABI changes

or is it fine as is?

Yes please, Node.js in text wherever possible.

Impressed that you're reviewing your own PR 😁.

gibfahn · 2017-04-23T10:21:10Z

COLLABORATOR_GUIDE.md

+cause breakage in the ecosystem. To test node ABI changes you can run [`citgm-abi-smoker`](https://ci.nodejs.org/job/citgm-abi-smoker/).
+
+* [`node-stress-single-test`](https://ci.nodejs.org/job/node-stress-single-test/)
+is designed to allow one to run a single test over and over on a specific platform


a single test -> a group of tests

Fixes: #12021 I made the suggested changes and created a navigation link to maintain consistency with other section headings.

gibfahn · 2017-04-23T11:27:42Z

@jasnell LGTY?

morrme · 2017-05-08T13:41:28Z

@gibfahn there are some conflicts now (i guess because something else was merged ahead of this?). looks like it's just my addition of this section to the TOC.

gibfahn · 2017-05-08T20:43:39Z

there are some conflicts now (i guess because something else was merged ahead of this?). looks like it's just my addition of this section to the TOC.

Thanks for the ping, looks like this got lost. Please could you rebase this on upstream/master and force push? Comment here when you've done that and I'll land.

Requested changes have been made.

morrme · 2017-05-09T18:34:15Z

@gibfahn no luck. I can try starting over if that will help.

mhdawson · 2017-05-09T19:28:30Z

@morrme what do you mean by "no luck". In the worst case you should be able to start a rebase which will fail and then you will need to manually resolve the conflict. At that point you should then be able to force push to this branch.

mhdawson · 2017-05-09T19:28:55Z

If you need help to walk through those steps just let me know.

morrme · 2017-05-09T20:39:10Z

@mhdawson It did not work for me. I also tried to resolve the conflict manually but that didn't work either.

I could try doing a new PR to get it done quickly...

gibfahn · 2017-05-09T22:25:14Z

@morrme I recommend doing this:

git fetch --all # Update all your remotes
git checkout doc-ci-jobs # Make sure you're on the right branch
git reset --hard @~ # drop the last commit (the merge one)
git rebase -i @~9  # rebase on HEAD~9 as you made 9 commits

Change the pick to s (i.e. squash) for all but the first commit, save and quit.

Edit the message to whatever you think the final message should be (what will be landed), save and quit.

git rebase upstream/master # Rebase on the upstream, should be easier now there's just one commit
git status # Shows you the file that has conflicts, in this case COLLABORATOR_GUIDE.md

Open the file in your editor of choice and fix the conflicts, save and quit:

Before:

After:

(Note that I kept the bit marked HEAD, and just added your new entry with the proper indentation).

git add COLLABORATOR_GUIDE.md
git rebase --continue
git status # Will show that your branch has diverged from origin/doc-ci-jobs
git push --force-with-lease # Overwrite your remote branch with the updated local one

Let me know if that works for you.

EDIT: I recommend trying to work through and understand the git squash and rebase workflow, it's pretty opaque initially, but once you get used to it it makes sense. Let me know if you have any questions.

mhdawson · 2017-05-11T13:25:05Z

@morrme if it helps I'm happy to arrange a time to do a hangout/zoom to work through it.

morrme · 2017-05-11T16:24:05Z

@mhdawson Thank you so much for that offer! I've tried to get involved as much as I can, but there's still so much I have to learn. I thought conflict was resolved now, though...? cc: @gibfahn

gibfahn · 2017-05-11T16:29:10Z

I thought conflict was resolved now, though...?

I'm afraid not, it looks like at some point you did a git pull or a git merge, hence the merge commit:

If you follow what I suggested it should work, you just have to remove the extra commit you added, I'll update that comment (EDIT: updated the instructions)

morrme · 2017-05-11T16:32:54Z

@gibfahn There was a button on the PR that allowed me to fix the resolution manually. I hadn't gotten anywhere with it but your instructions actually helped that too. That's the merge commit you see from 2 days ago.

gibfahn · 2017-05-11T16:43:04Z

There was a button on the PR that allowed me to fix the resolution manually. I hadn't gotten anywhere with it but your instructions actually helped that too. That's the merge commit you see from 2 days ago.

The problem with that is that the CI will fail if you use the web UI to fix issues, as it rebases onto the latest master (to make sure there aren't any incompatibilities). IMO this is an issue with the Fix conflicts button. Point is that if you could use the command line that'd be great.

morrme · 2017-05-11T17:13:21Z

@gibfahn Sorry about that. Well I've closed this one, perhaps someone else will come across the issue and make better headway! Thanks to you and @mhdawson (and everyone else!!!) for your support!

mhdawson · 2017-05-11T22:10:37Z

@morrme, at this point I'd say feel free to submit a new PR with your changes. Somebody else could pick it up it but since you've done the work to get it this far, it would be nice to see it be tagged with your name.

gibfahn · 2017-05-12T13:53:21Z

I applied your commits to a branch on my fork, rebased and squashed.

Branch: https://github.com/gibfahn/node/tree/doc-ci-jobs
Linter CI: https://ci.nodejs.org/job/node-test-linter/9015/ EDIT: Linter passed, landing...

gibfahn · 2017-05-12T13:57:19Z

Landed in 1d5f5aa , thanks for your contribution @morrme !

PR-URL: #12555 Fixes: #12021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

morrme · 2017-05-12T16:10:33Z

@gibfahn & @mhdawson Thanks so much! Sorry to be so gunshy, I just didn't want to screw anything up. I actually thought about going through the steps this morning before seeing this land, since I was OK with losing out on the commit either way. It was a confidence problem, embarrassingly.

mhdawson · 2017-05-12T16:16:38Z

@morrme thank you for your contribution. We make good progress on Node.js only because of the contributions of a broad range of people like yourself.

gibfahn · 2017-05-12T16:58:16Z

@morrme in general the worst that can happen is you lose your changes, so in general feel free to try things (history is usually recoverable anyway).

If you want to contribute again feel free!

PR-URL: nodejs#12555 Fixes: nodejs#12021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #12555 Fixes: #12021 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

morrme added 3 commits April 19, 2017 00:51

Update COLLABORATOR_GUIDE.md

5788c98

Update COLLABORATOR_GUIDE.md

e8897e0

update job definitions

bc520c7

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 20, 2017

jasnell approved these changes Apr 20, 2017

View reviewed changes

mscdex added the meta Issues and PRs related to the general management of the project. label Apr 20, 2017

gibfahn suggested changes Apr 21, 2017

View reviewed changes

doc: update COLLABORATOR_GUIDE.md

c94b070

Fixes: #12021

gibfahn approved these changes Apr 21, 2017

View reviewed changes

mhdawson approved these changes Apr 21, 2017

View reviewed changes

richardlau reviewed Apr 21, 2017

View reviewed changes

morrme added 3 commits April 21, 2017 13:25

Update COLLABORATOR_GUIDE.md

7642551

doc: update COLLABORATOR_GUIDE.md

c23bc24

Fixes: #12021

doc: remove node-test-commit from CI jobs list

696e2aa

Fixes: #12555

richardlau approved these changes Apr 21, 2017

View reviewed changes

jasnell previously requested changes Apr 21, 2017

View reviewed changes

lpinca approved these changes Apr 22, 2017

View reviewed changes

doc: additional changes to COLLABORATOR_GUIDE.md

c18f0b9

Fixes: #12021

morrme commented Apr 22, 2017

View reviewed changes

morrme changed the title ~~documentation: add descriptions of CI jobs~~ doc: add descriptions of CI jobs Apr 22, 2017

gibfahn reviewed Apr 23, 2017

View reviewed changes

doc: update CI job descriptions

31ce401

Fixes: #12021 I made the suggested changes and created a navigation link to maintain consistency with other section headings.

jasnell approved these changes May 9, 2017

View reviewed changes

Merge branch 'master' into doc-ci-jobs

5f0eb87

morrme closed this May 11, 2017

gibfahn reopened this May 12, 2017

gibfahn closed this May 12, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the land-on-v6.x label Jun 22, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

doc: add descriptions of CI jobs #12555

doc: add descriptions of CI jobs #12555

Conversation

morrme commented Apr 20, 2017 • edited Loading

Checklist

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Apr 21, 2017

gibfahn commented Apr 21, 2017

morrme commented Apr 21, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

lpinca left a comment

Choose a reason for hiding this comment

morrme Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Apr 23, 2017

morrme commented May 8, 2017

gibfahn commented May 8, 2017 • edited Loading

morrme commented May 9, 2017

mhdawson commented May 9, 2017

mhdawson commented May 9, 2017

morrme commented May 9, 2017

gibfahn commented May 9, 2017 • edited Loading

Before:

After:

mhdawson commented May 11, 2017

morrme commented May 11, 2017

gibfahn commented May 11, 2017 • edited Loading

morrme commented May 11, 2017

gibfahn commented May 11, 2017

morrme commented May 11, 2017

mhdawson commented May 11, 2017 • edited Loading

gibfahn commented May 12, 2017 • edited Loading

gibfahn commented May 12, 2017

morrme commented May 12, 2017

mhdawson commented May 12, 2017

gibfahn commented May 12, 2017

morrme commented Apr 20, 2017 •

edited

Loading

gibfahn Apr 21, 2017 •

edited

Loading

gibfahn Apr 22, 2017 •

edited

Loading

morrme Apr 22, 2017 •

edited

Loading

gibfahn commented May 8, 2017 •

edited

Loading

gibfahn commented May 9, 2017 •

edited

Loading

gibfahn commented May 11, 2017 •

edited

Loading

mhdawson commented May 11, 2017 •

edited

Loading

gibfahn commented May 12, 2017 •

edited

Loading