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

Closed

Conversation

gireeshpunathil
Copy link
Contributor

@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

Choose a reason for hiding this comment

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

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
Contributing.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Contributor Author

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

@gireeshpunathil
Copy link
Contributor Author

@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
Contributor

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
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

@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
Contributing.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Contributor Author

@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
Contributor Author

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

@gireeshpunathil
Copy link
Contributor Author

@dougwilson - I just squashed all the commits into one

@dougwilson
Copy link
Contributor

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
Copy link
Contributor Author

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

@dougwilson
Copy link
Contributor

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
Contributor Author

@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
Contributor

  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).

@gireeshpunathil
Copy link
Contributor Author

@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
Contributor

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
Contributor Author

@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
Contributor

Hi @gireeshpunathil that issue is orthogonal to this issue.

@gireeshpunathil
Copy link
Contributor Author

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;

@dougwilson
Copy link
Contributor

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
Contributor Author

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

@gireeshpunathil
Copy link
Contributor Author

in general, this can have more reviews!

@crandmck
Copy link
Member

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
Contributor Author

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
Contributor

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

@wesleytodd
Copy link
Member

Closing in favor of #5484.

@wesleytodd wesleytodd closed this Feb 16, 2024
wesleytodd pushed a commit that referenced this pull request Feb 16, 2024
wesleytodd pushed a commit that referenced this pull request Feb 18, 2024
dougwilson pushed a commit that referenced this pull request Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants