X Tutup
The Wayback Machine - https://web.archive.org/web/20201107150935/https://github.com/expressjs/express/pull/4210
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 section for project captains #4210

Open
wants to merge 1 commit into
base: master
from

Conversation

@gireeshpunathil
Copy link
Member

@gireeshpunathil gireeshpunathil commented Mar 9, 2020

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Copy link
Member

@wesleytodd wesleytodd left a comment

Can we also add the part about the TC and existing repo captains having a vote?

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
and the registry, as well as in providing user support.

To become a captain for a project you will be expected to participate in that
project for at least 6 months prior to the request. You should have helped with

This comment has been minimized.

@dougwilson

dougwilson Mar 10, 2020
Member

Should this be "for at least 6 months in the role of committer"?

This comment has been minimized.

@gireeshpunathil

gireeshpunathil Mar 10, 2020
Author Member

we did not specifically call this out in the TC. Should we? because there is an approval process, adding such an additional criteria may be seen as redundant?

This comment has been minimized.

@dougwilson

dougwilson Mar 13, 2020
Member

My comment is because I assume that in order to actually function as a repo owner one would need to be a committer? Because if a user had never been a committer before and suddenly was a repo owner, that would seem odd, as one would think that they should at least have non-zero experience landing commits into the repo?

This comment has been minimized.

@dougwilson

dougwilson Mar 13, 2020
Member

Anyway, the main reasons why it jumped out to me is that the TC is a progression of committer -> TC member, so thought that maybe that would make sense for Repo Captain as well. It just felt natural for some reason, which is the only reason I asked. It also felt like it may help better clarify what "participate" meant if that came up in the future as a point of content (did X actually "participate" for 6 months, etc.). I just felt like if the pre-req felt less fuzzy, a user would be more confident requesting the role by better knowing if they met it or not, idk, haha

This comment has been minimized.

@dougwilson

dougwilson Mar 24, 2020
Member

So are there no comments on this still? If not, I think the wording should be clarified in this manor. For example, in the TC section, it says that committers elect other committers to the TC. If we do not spell out somewhere that a repo captain is also a committer, then it is not clear that a repo captain can be a TC member unless they also separately become a committer.

I think we should clarify that repo captains are a subset of committers, just like TC members are.

Contributing.md Show resolved Hide resolved
@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Mar 20, 2020

@dougwilson , @wesleytodd : we are ready to land this, isn't it? what is blocking this?

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Mar 23, 2020

@dougwilson / @wesleytodd - can you either remove the red-X or clarify what your concern is? If it is just that one is waiting for other's approval, please state so, so that we can move forward.

/cc @expressjs/express-tc - FYI, and important PR for defining project captains for repos. Please chime in if you have comments, or else complete review to help progress!

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Mar 23, 2020

There is a simple issue here I was going to fix on merge, but didn't get a chance to this weekend with what is going on in my country. After work today I will at least comment on it so anyone can make the change if I cannot.

Copy link
Member

@dougwilson dougwilson left a comment

@gireeshpunathil these are my notes that I am meaning to fix, but as per your request on what the hold up is, I have written them down here in case you wanted to fix them. I am willing to do so, though. LMK what you want to do.

Contributing.md Outdated Show resolved Hide resolved
and the registry, as well as in providing user support.

To become a captain for a project you will be expected to participate in that
project for at least 6 months prior to the request. You should have helped with

This comment has been minimized.

@dougwilson

dougwilson Mar 24, 2020
Member

So are there no comments on this still? If not, I think the wording should be clarified in this manor. For example, in the TC section, it says that committers elect other committers to the TC. If we do not spell out somewhere that a repo captain is also a committer, then it is not clear that a repo captain can be a TC member unless they also separately become a committer.

I think we should clarify that repo captains are a subset of committers, just like TC members are.

Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Mar 24, 2020

@dougwilson - I have addressed all the review comments, mostly following your suggestions AS IS. Please have a look and do the needful!

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 4, 2020

it has been 11 days since the last update. can we take this forward?

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 9, 2020

@dougwilson - I just squashed all the commits into one

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 9, 2020

Thanks @gireeshpunathil ! As we discussed on the TC meeting, it should be in the format as the current commit on master and our guide: the message on the first line, a blank line, a reference to the PR in the form "closes #PR". I can make the said changes if you need. LMK

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:repocaptains branch from f628080 to 1d73579 Apr 9, 2020
@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 9, 2020

@dougwilson - made the changes to the commit message as per the guidelines; PTAL.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 15, 2020

Ok... It still does not seem to be in the form from my comment... ? I can just update if you need, let me know. If I can better clarify how the message should be constructed let me know as well. I believe it was clear, and provided an example of a previous comment message, but if I can provide better guidance for the format, I certainly can.

I am circling around on this because I would like this to get merged, but I recall that you wanted to do the commit editing so I could just do the fast-forward merge, but it's not in the correct state for that. I can make those edits if you like, but don't want to force push on your branch without you being aware as per our previous conversation :)

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 15, 2020

@dougwilson -

  • thanks - yes, I prefer to be advised over edits if possible, towards improving my own knowledge on those areas.

this is the current commit message:

doc: add section for project captains
    
closes https://github.com/expressjs/discussions/issues/98
Co-Authored-By: Wes Todd <wes@wesleytodd.com>

this is the rule:
the message on the first line, a blank line, a reference to the PR in the form "closes #PR".

  • the message is on the first line
  • there is a blank line
  • there is a reference to the PR against closes verb

the two additional things are:

  • there is a subsystem (doc) in the message line (following the existing conventions)
  • there is a co-author field (attribution of authorship, many of the original wordings came from Wes)

please advise where is the change required?

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 15, 2020

  1. The subsystem is "docs" and not "doc"
  2. The closes should be closes #4210
  3. We do not include co-author fields, but we can if desired to amend our guidelines, in which this commit would wait for that to happen

It should just be something like

docs: add project captains to contribution

closes #4210

Then as discussed in the TC meeting, the committer on the commit would need to be the person performing the merge itself (myself unless you would like to wait for another to do that).

closes #4210
@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:repocaptains branch from 1d73579 to 8976172 Apr 15, 2020
@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 15, 2020

@dougwilson - adjusted the commit message accordingly, PTAL!
@wesleytodd - I had earlier kept you as a co-author for this PR, but looks like we do not include that as per the current process. Can you please review and provide your concurrence? If you have concerns pls let me know, we would need to go the route of amending guidelines.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 15, 2020

So based on the conversation going on in expressjs/discussions#121 it seems like this is not ready to land as-is, as it seems that there may be either additional discussion to have on the goal here and/or updating to the wording in this pull request to reflect it. Specifically in regards to what falls under repo ownership.

It would come to reason to me that landing pull requests would be a "day-to-day" task for said project captains, and their particular merging strategies, commit message formats, etc. would serve the repo on a technical front, as outlined in this document.

I think part of the goal of project captains is to empower them to manage their repos, and commit messages seems to fall directly in that category to me. For example, if a project captain wants to use something like standard-release to automate releases and history file generation, they may need specific structure to commit messages. We should strive to empower the owners to do what they think will improve their workflow.

But maybe I'm misunderstanding on that point. Especially if I'm misunderstanding, that definately means we need greater clarity in the document, as we don't want to have everyone interpreting it differently, I presume. I'm happy to provide suggested edits on the wording, but likely need input from the original author @wesleytodd to sync on the interpretation so I can help clarify 👍

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 15, 2020

@dougwilson - it seems strange (and funny) to me that you closed expressjs/discussions#121 with no action on it, and then quoting its content as a reason for further work here!

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 15, 2020

Hi @gireeshpunathil that issue is orthogonal to this issue.

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 15, 2020

But maybe I'm misunderstanding on that point. Especially if I'm misunderstanding, that definately means we need greater clarity in the document, as we don't want to have everyone interpreting it differently, I presume.

here is the sequence of events, for better clarity for everyone:

  • the commit message did not follow this repo's convention
  • more specifically, I added Co-authored-by field, but not not an established practice in this repo
  • @dougwilson pointed out that, I promptly removed it, but sought verbal consent from the co-author
  • I opened an issue to discuss that: specifically, implementing a guideline pan-project
  • that is closed as inappropriate in that repo, and to be dealt in individual repos.
  • now back here, @dougwilson says it is causing misunderstanding!

This is becoming a surprise to me! where is the mis-understanding? If you assert that no org wide guidelines are present, so be it, and I will seek change proposal with captains, period! where is it that I interpreted this document at all? leave alone interpreted differently?

I suggest we take a step back;

  • retire the discussion in expressjs/discussions#121 as it is already closed and I agreeing to your proposal to take anything further with individual repo captians
  • and go ahead with this proposal
@LinusU
LinusU approved these changes Apr 17, 2020
@dougwilson
Copy link
Member

@dougwilson dougwilson commented Apr 17, 2020

Hi @gireeshpunathil please let me know if there is some time in which you are available for a voice chat; I would like to proceed with landing this PR, but I am honestly unsure at this point how to interact with your pull requests that will not come off in bad faith, and would like a chance to have a discussion with you so we are on the same page so we can make progress landing.

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Apr 17, 2020

@dougwilson - I sent you a mail with my time preferences.

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Jun 28, 2020

in general, this can have more reviews!

@crandmck
Copy link
Member

@crandmck crandmck commented Jul 26, 2020

I don't have any real comments on this PR, but once the discussion is resolved and it lands, we should make sure the change is reflected on the website; I opened expressjs/expressjs.com#1184 to track that.

@gireeshpunathil
Copy link
Member Author

@gireeshpunathil gireeshpunathil commented Jul 27, 2020

I believe I have addressed all the outstanding comments. pinging @expressjs/express-tc , to seek some reviews or take it towards landing.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 27, 2020

I am locking this PR until the above issue with the interaction between myself and @gireeshpunathil is resolved.

@expressjs expressjs locked as too heated and limited conversation to collaborators Jul 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.
X Tutup