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: update landing pr info in onboarding doc #8344

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Member

Trott commented Aug 30, 2016

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

doc

Description of change

Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

@Trott Trott added the doc label Aug 30, 2016

@mscdex mscdex added the meta label Aug 30, 2016

@targos

View changes

Show outdated Hide outdated doc/onboarding.md
* `#node-dev` on freenode
* force-pushing to fix things after is allowed for ~10 minutes, be sure to notify people in IRC if you need to do this, but avoid it
* Info on PRs that don't like to apply found under [**"If `git am` fails"**](./onboarding-extras.md#if-git-am-fails).
* Force-pushing to fix things after is allowed for ~10 minutes. Avoid it if you can. Post to `#node-dev` (IRC) if you need to force push.

This comment has been minimized.

@targos

targos Aug 30, 2016

Member

Should we mention the --force-with-lease option here ?

@targos

targos Aug 30, 2016

Member

Should we mention the --force-with-lease option here ?

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 30, 2016

Member

Last part can probably be it's own bullet?

@Fishrock123

Fishrock123 Aug 30, 2016

Member

Last part can probably be it's own bullet?

This comment has been minimized.

@Trott

Trott Aug 30, 2016

Member

I think --force-with-lease with lease is worth mentioning and will add it.

Happy to make the last part its own sub-bullet. I don't have a strong opinion either way.

@Trott

Trott Aug 30, 2016

Member

I think --force-with-lease with lease is worth mentioning and will add it.

Happy to make the last part its own sub-bullet. I don't have a strong opinion either way.

@evanlucas

View changes

Show outdated Hide outdated doc/onboarding.md
* one "LGTM" is usually sufficient, except for semver-major changes
* the more the better
* semver-major (breaking) changes must be reviewed in some form by the CTC
* One "LGTM" is usually sufficient, except for semver-major changes.

This comment has been minimized.

@evanlucas

evanlucas Aug 30, 2016

Member

Should we be explicit about this being as long as there are no objections?

@evanlucas

evanlucas Aug 30, 2016

Member

Should we be explicit about this being as long as there are no objections?

This comment has been minimized.

@Trott

Trott Aug 30, 2016

Member

The "as long as there are no objections" stuff is covered elsewhere, but it sure wouldn't hurt to repeat it here. I'll do that.

@Trott

Trott Aug 30, 2016

Member

The "as long as there are no objections" stuff is covered elsewhere, but it sure wouldn't hurt to repeat it here. I'll do that.

* `#node-dev` on freenode
* force-pushing to fix things after is allowed for ~10 minutes, be sure to notify people in IRC if you need to do this, but avoid it
* Info on PRs that don't like to apply found under [**"If `git am` fails"**](./onboarding-extras.md#if-git-am-fails).

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 30, 2016

Member

This is still important

edit: nvm, see it below...

@Fishrock123

Fishrock123 Aug 30, 2016

Member

This is still important

edit: nvm, see it below...

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 31, 2016

Member

Nits addressed (I hope). PTAL

Member

Trott commented Aug 31, 2016

Nits addressed (I hope). PTAL

* no one (including TSC or CTC members) pushes directly to master without review
* an exception is made for release commits only
* No one (including TSC or CTC members) pushes directly to master without review.
* An exception is made for release commits only.

This comment has been minimized.

@jasnell

jasnell Aug 31, 2016

Member

Or corrections to the commit log within the 10 minute window?

@jasnell

jasnell Aug 31, 2016

Member

Or corrections to the commit log within the 10 minute window?

This comment has been minimized.

@Trott

Trott Aug 31, 2016

Member

Technically, I suppose yes. No one has ever been confused about that, as far as I know, though. And I'd prefer to avoid introducing content that my increase cognitive load without providing valuable information. If you want to propose some language that you think would work, I'm open to it.

@Trott

Trott Aug 31, 2016

Member

Technically, I suppose yes. No one has ever been confused about that, as far as I know, though. And I'd prefer to avoid introducing content that my increase cognitive load without providing valuable information. If you want to propose some language that you think would work, I'm open to it.

@jasnell

View changes

Show outdated Hide outdated doc/onboarding.md
* One `LGTM` is sufficient, except for semver-major changes.
* More than one is better.
* Breaking changes must be LGTM'ed by at least two CTC members.
* If one or more Collaborators object to a change, it may not land. Your options in such a situation (aside from abandoning the change) are:

This comment has been minimized.

@jasnell

jasnell Aug 31, 2016

Member

I would soften the language a bit... Perhaps

* If one or more Collaborators object to a change, it should not land until 
those objections can be handled. The options for such a situation include:
  * Engaging those with objections to determine a viable path forward;
  * Altering the pull request to address those objections;
  * Or escalating the discussion to the CTC using the `ctc-agenda` label. 
    This, however, should be a last step only if other approaches are
    unsuccessful.
@jasnell

jasnell Aug 31, 2016

Member

I would soften the language a bit... Perhaps

* If one or more Collaborators object to a change, it should not land until 
those objections can be handled. The options for such a situation include:
  * Engaging those with objections to determine a viable path forward;
  * Altering the pull request to address those objections;
  * Or escalating the discussion to the CTC using the `ctc-agenda` label. 
    This, however, should be a last step only if other approaches are
    unsuccessful.

This comment has been minimized.

@Trott

Trott Aug 31, 2016

Member

I've (mostly) adopted that language. Thanks!

@Trott

Trott Aug 31, 2016

Member

I've (mostly) adopted that language. Thanks!

@jasnell

View changes

Show outdated Hide outdated doc/onboarding.md
* Wait before merging non-trivial changes.

This comment has been minimized.

@jasnell

jasnell Aug 31, 2016

Member

It might be good to clarify some differences between trivial and non-trivial

@jasnell

jasnell Aug 31, 2016

Member

It might be good to clarify some differences between trivial and non-trivial

@jasnell

View changes

Show outdated Hide outdated doc/onboarding.md
* **Run the PR through CI before merging!**
* An exception can be made for documentation-only PRs.

This comment has been minimized.

@jasnell

jasnell Aug 31, 2016

Member

There's an exception here for addon documentation changes, since the examples in the addons.md are run through CI.

@jasnell

jasnell Aug 31, 2016

Member

There's an exception here for addon documentation changes, since the examples in the addons.md are run through CI.

This comment has been minimized.

@Trott

Trott Aug 31, 2016

Member

Don't I know it! I figured I wouldn't worry about mentioning that detail in the onboarding doc but, yeah, it probably should be in there. (My preferred alternative--insist that everything go through CI, no exceptions--would probably not get consensus.)

@Trott

Trott Aug 31, 2016

Member

Don't I know it! I figured I wouldn't worry about mentioning that detail in the onboarding doc but, yeah, it probably should be in there. (My preferred alternative--insist that everything go through CI, no exceptions--would probably not get consensus.)

Trott added some commits Aug 30, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 31, 2016

Member

Comments addressed. PTAL again.

Member

Trott commented Aug 31, 2016

Comments addressed. PTAL again.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

LGTM!

Member

jasnell commented Aug 31, 2016

LGTM!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

Thanks for doing this, btw

Member

jasnell commented Aug 31, 2016

Thanks for doing this, btw

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Sep 1, 2016

Member

lgtm

Member

evanlucas commented Sep 1, 2016

lgtm

Trott added a commit to Trott/io.js that referenced this pull request Sep 1, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: nodejs#8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 1, 2016

Member

Landed in 2501a38

Member

Trott commented Sep 1, 2016

Landed in 2501a38

@Trott Trott closed this Sep 1, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: nodejs#8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: #8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

MylesBorins added a commit that referenced this pull request Sep 30, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: #8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: #8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: #8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

doc: update landing pr info in onboarding doc
Reword and re-organize. Only significant content change is to
specifically call out the two-CTC-member-LGTM requirement for
semver-major changes.

PR-URL: #8344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

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