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

doc: add POST_STATUS_TO_PR info to onboarding doc #8059

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 10, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

POST_STATUS_TO_PR did not used to work.

Now it works.

Update the onboarding documentation accordingly.

@Trott Trott added the doc Issues and PRs related to the documentations. label Aug 10, 2016
@addaleax
Copy link
Member

Commit message typo: did not used to

LGTM but got a question… does it ever make sense not to check that box?

@Trott
Copy link
Member Author

Trott commented Aug 10, 2016

@addaleax (Whoops, ignore previous comment, I misread your question.) I guess not. I suppose it might be worthwhile asking @nodejs/build to make that box checked by default and then we don't have to mention it at all in the onboarding doc!

`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.
@Trott
Copy link
Member Author

Trott commented Aug 10, 2016

@addaleax Updated the commit message. Thanks.

@Fishrock123
Copy link
Contributor

@jbergstroem think we are ready to move forward with this or is there still stuff being worked on there?

@fhinkel
Copy link
Member

fhinkel commented Aug 11, 2016

Can we change the default for POST_STATUS_TO_PR to be checked? Seems easier than to add an extra instruction to the onboarding doc about actively turning it on.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2016

Yeah, I'd definitely prefer for post status to be on by default.

@jbergstroem
Copy link
Member

I'd like to get some more time working with this if that's ok. ETA ~1w.

@mhdawson
Copy link
Member

Same goes for me for having in on by default once we are ready.

Trott added a commit to Trott/io.js that referenced this pull request Aug 12, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: nodejs#8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member Author

Trott commented Aug 12, 2016

Landed in fa1476c.

(We can update it again when the default switches to the box being checked. I expect to be updating the onboarding doc frequently anyway.)

@Trott Trott closed this Aug 12, 2016
cjihrig pushed a commit that referenced this pull request Aug 15, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: #8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig mentioned this pull request Aug 15, 2016
MylesBorins pushed a commit that referenced this pull request Sep 9, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: #8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: #8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: #8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
`POST_STATUS_TO_PR` previously did not work.

Now it works.

Update the onboarding documentation accordingly.

PR-URL: #8059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
@Trott Trott deleted the itsalive111 branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants