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.

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

doc: remove confusing reference in governance doc #9073

Closed
wants to merge 5 commits into from
Closed

doc: remove confusing reference in governance doc #9073

wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 13, 2016

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

doc meta

Description of change

At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link.

/cc @nodejs/ctc @thefourtheye

I don't think this needs to go on the CTC agenda because it is not a change to our governance process, but if anyone else feels differently, feel free to add the ctc-agenda label.

@Trott
doc: remove confusing reference in governance doc
7b6deb1 
At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link.
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 13, 2016
@Trott Trott added doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. and removed meta Issues and PRs related to the general management of the project. labels Oct 13, 2016
@MylesBorins
Copy link
Contributor

LGTM

thefourtheye
thefourtheye approved these changes Oct 13, 2016
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the text unambiguous, but now how the consensus is reached has to be explained I think.

@jasnell
Copy link
Member

jasnell commented Oct 13, 2016

while I agree that it's confusing as is, I'm not sure if this change helps.

@mhdawson
Copy link
Member

I agree we do need some additional info on what consensus seeking means.

@Trott
Copy link
Member Author

Trott commented Oct 14, 2016

OK, I added new material to explain our process. This hopefully addresses the concerns of @jasnell and @mhdawson. Please take a look.

I chose to eliminate the word consensus from this particular passage because it is a bit jargon-y in this context. I think spelling out our process in specifics (as I hope I have accurately done in the bullet points) is more useful in a document like this.

thefourtheye
thefourtheye reviewed Oct 14, 2016
View reviewed changes
GOVERNANCE.md
* discussion and/or additional changes result in no Collaborators objecting to
the change; previously-objecting Collaborators do not necessarily have to
sign-off on the change, but they should not be opposed to it
* the change is escalated to the CTC and the CTC approves the change; this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be considered as approval by CTC? I mean any of the CTC signs off or more than one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess for something as significant as resolving an impasse on a controversial change, we should require a vote. I'll update the text to say that. We can always change the rules if that turns out to be onerous. But it seems that this comes up relatively infrequently.

GOVERNANCE.md
for sign-off.

If there is disagreement among Collaborators about whether a proposed change
should be accepted, then the change may not be accepted unless:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this sound okay? I am finding it very difficult to understand the double negatives and the meaning of this sentence

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can probably be improved. Let me try to revise it...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better?

If one or more Collaborators oppose a proposed change, then the change can not
be accepted unless:

@Trott
Copy link
Member Author

Trott commented Oct 14, 2016

Made some clarifications based on @thefourtheye's questions and pushed. PTAL

@Trott
Copy link
Member Author

Trott commented Oct 15, 2016

@nodejs/ctc

@thefourtheye
Copy link
Contributor

LGTM

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@mhdawson
Copy link
Member

LGTM

jasnell
jasnell reviewed Oct 19, 2016
View reviewed changes
GOVERNANCE.md
sign-off on the change, but they should not be opposed to it
* the change is escalated to the CTC and the CTC votes to approve the change;
this should be used only after other options (especially discussion among
the disagreeing Collaborators) have been exhausted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: Can you capitalize the first letter in the bullet points and add appropriate punctuation at the end of each.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

@Trott Trott removed the ctc-review label Oct 19, 2016
Trott added a commit to Trott/io.js that referenced this pull request Oct 19, 2016
@Trott
doc: remove confusing reference in governance doc
15b4642 
At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link and replaced it with a text explanation.

PR-URL: nodejs#9073
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 19, 2016

Landed in 15b4642

@Trott Trott closed this Oct 19, 2016
jasnell pushed a commit that referenced this pull request Oct 20, 2016
@Trott @jasnell
doc: remove confusing reference in governance doc
38cf1d4 
At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link and replaced it with a text explanation.

PR-URL: #9073
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@Trott
doc: remove confusing reference in governance doc
3ee94f2 
At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link and replaced it with a text explanation.

PR-URL: #9073
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@Trott
doc: remove confusing reference in governance doc
3b839d1 
At the CTC meeting today, Sakthipriyan noted that there was a link to
the CTC consensus material from the pull request consensus material. The
link was confusing because the CTC consensus material is
meeting-specific, which does not apply to pull requests. I have removed
that link and replaced it with a text explanation.

PR-URL: #9073
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Nov 22, 2016
Merged
Merged
@Trott Trott deleted the meeting-practices branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @MylesBorins @jasnell @mhdawson @thefourtheye @nodejs-github-bot