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

[WIP] review of project governance #1540

Merged
merged 6 commits into from Aug 11, 2016
Merged

[WIP] review of project governance #1540

merged 6 commits into from Aug 11, 2016

Conversation

joeyaiello
Copy link
Contributor

@joeyaiello joeyaiello commented Jul 27, 2016

Please don't merge this PR yet

  • Resolves the need for an all-up governance doc.
  • Code is up-to-date with master.
  • [N/A] Add tests that fail without your changes.
  • Update all relevant documentation.

This is purely a WIP review to make sure I'm not completely out of left field here. I still need to do a considerable rewrite of the other contributor docs in order to change terminology to align with this document (e.g. maintainers.md, contributing.md, etc.)

EDIT: Thanks to @TravisEz13 and @KarolKaczmarek for giving me some existing documentation, governance, and structure to work with. :)

If you are new to Git, we recommend you start by reviewing our
[Git basics document][git-basics] where you will find Git installation
instructions, cheat sheets and links to our favorite Git tutorials. We also
recommend, reviewing an example of a [basic Git commit walkthrough][git-commit].
Copy link
Member

Choose a reason for hiding this comment

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

This section is duplicated but different than what is in contributing.md. Perhaps this should be a sub-section and contributing should link back here or the other way around.

@joeyaiello joeyaiello force-pushed the joey/governance branch 2 times, most recently from 3c93421 to d059425 Compare July 27, 2016 18:48
@joeyaiello
Copy link
Contributor Author

joeyaiello commented Jul 27, 2016

Didn't meant to include a bunch of those edits in my weak attempt at a rebase. My apologies. I'm inline with master now #Resolved


### PowerShell Committee Membership

Copy link
Member

Choose a reason for hiding this comment

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

The entire time I was reading I was wondering where this section was. #Resolved

Copy link
Contributor Author

@joeyaiello joeyaiello Jul 27, 2016

Choose a reason for hiding this comment

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

So you think it should be moved up? Actually received feedback to move it down, but I'm open to more suggestion. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

yeah, because basically I don't know what you are talking about the whole time.


In reply to: 72535762 [](ancestors = 72535762)

Copy link
Contributor Author

@joeyaiello joeyaiello Jul 28, 2016

Choose a reason for hiding this comment

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

Done #Resolved

@TravisEz13
Copy link
Member

🕐

@TravisEz13
Copy link
Member

TravisEz13 commented Jul 28, 2016

1 comments do not have a response. #Pending

@joeyaiello joeyaiello force-pushed the joey/governance branch 3 times, most recently from 7a7f948 to 6f5737d Compare August 4, 2016 20:38
1. **DO** ensure that contributors [write Pester tests][pester] for all new/changed functionality
1. **DO** ensure that contributors [write documentation][docs-contributing] for all new-/changed functionality
1. **DO** encourage contributors to refer to issues in their pull request description per the [issue template](../../.github/ISSUE_TEMPLATE.md) (e.g. `Resolves issue #123`)
Copy link
Member

Choose a reason for hiding this comment

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

This should be per the pull request template

@TravisEz13
Copy link
Member

TravisEz13 commented Aug 9, 2016

See [this][closing-via-message] for more details.

This does not agree with the maintainers readme.md
`SHOULD encourage contributors to refer to issues in their pull request description ...'

Either this should say that they should open a new issues if one does not exist, or the maintainers readme should be qualified, with if the issue already exists #Pending


Refers to: .github/CONTRIBUTING.md:101 in ee5a755. [](commit_id = ee5a755540086ff5475a6b67a1330f16b49a83c0, deletion_comment = False)

@TravisEz13
Copy link
Member

TravisEz13 commented Aug 9, 2016

  • Otherwise, these issues should be treated like any other issue in this repo.

There have been comments that doc issues should not require opening an issue. Example #Pending


Refers to: .github/CONTRIBUTING.md:51 in ee5a755. [](commit_id = ee5a755540086ff5475a6b67a1330f16b49a83c0, deletion_comment = False)

@TravisEz13 TravisEz13 mentioned this pull request Aug 9, 2016
4 tasks
@joeyaiello
Copy link
Contributor Author

joeyaiello commented Aug 9, 2016

Had an offline conversation with @TravisEz13 about issues being created prior to pull requests. I'm making some final modifications to this PR that should communicate the following:

  • issues should be created prior to doing large pieces of work. This is to try and minimize the duplication of effort on significant bodies of work
  • a PR should not be blocked on whether an issue exists. If they've done the work, and the work is sound, and the PR meets all the bars to being accepted by a repository maintainer, it should be accepted. Any discussion on it can happen in the PR.
  • however, that PR submitter should be reminded that, next time, they should start an issue to get this discussion and approval out of the way BEFORE they do a bunch of work. This is to minimize duplication of efforts and to minimize having to redo work that didn't get discussed or approved

Sound okay? #Resolved

@joeyaiello
Copy link
Contributor Author

Also, we need to merge this guy. We can continue to make any minor fixes over the next couple days, but it's gotta go in.

joeyaiello added 5 commits August 9, 2016 16:42
still need to do a considerable rewrite of other
contributor docs in order to change terminology
to align with governance.md
Now that we've gotten enough of a sign-off
from everyone involved in the governance
process, the docs need to be reworked to use a
consistent terminology set, links, and directory
structure.
@joeyaiello
Copy link
Contributor Author

joeyaiello commented Aug 9, 2016

Alright, @TravisEz13, everything should be merged up here. Can I get a signoff and merge from you? Thanks! #Pending

@alexandair
Copy link
Contributor

That section about issues prior to PR applies to code-related PR? Or, docs-related PRs too?

@joeyaiello
Copy link
Contributor Author

@alexandair the spirit of the guideline is, again, to minimize duplication of efforts and churn in one place. It's a poor-man's work tracking mechanism.

But given the spirit of the rule, you don't need to go file an issue for every typo. However, I do think there's some benefit to saying "hey, semantic line feeds are busted all over the repo, I'm going to go tackle that" and just giving people a heads up before changing 15-20 files. And there's certainly benefit to saying "hey, I'm going to go move a bunch of files around" or "I'm going to go refactor these sets of docs and move headers and content between files/folders". I say this from personal experience: this PR was super hard to get merged because of how much a moving target master was. (And not just the technical aspects of rebasing, but conceptually keeping track of often undiscussed decisions people were codifying throughout the maintainer and contributor documentation.)

I know it feels like needless process now, but I'd rather err on the side of a little process before we open the floodgates.

Plus, I also don't expect contributor documentation here to be changing significantly after the first month we're live. So, to be honest, I think it's kind of a no-op.

@TravisEz13 TravisEz13 added the Area-Maintainers-Documentation specific to documentation in this repo label Aug 10, 2016
1. **SHOULD** ask people to resend a pull request, if it [doesn't target `master`](../../.github/CONTRIBUTING.md#lifecycle-of-a-pull-request)
1. **SHOULD** wait for the [CI system][ci-system] build to pass for pull requests
(unless, for instance, the pull request is being submitted to fix broken CI)
1. **SHOULD** encourage contributors to refer to issues in their pull request description per the [issue template](../../.github/ISSUE_TEMPLATE) (e.g. `Resolves issue #123`).
Copy link
Member

Choose a reason for hiding this comment

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

he [issue template](. [](start = 95, length = 21)

Should be PR template

@TravisEz13
Copy link
Member

This looks good to me other than, this comment: #1540 (diff)

We should merge and file an issue for this.

@joeyaiello
Copy link
Contributor Author

Looks like I forgot to do it in both places. Should be fixed now

@TravisEz13 TravisEz13 merged commit cf23084 into master Aug 11, 2016
@TravisEz13 TravisEz13 deleted the joey/governance branch August 11, 2016 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Maintainers-Documentation specific to documentation in this repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants