Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Maintainer's guide, et al. #726

Merged
merged 5 commits into from
Jun 7, 2017
Merged

Maintainer's guide, et al. #726

merged 5 commits into from
Jun 7, 2017

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Jun 6, 2017

What does this do / why do we need it?

This adds a maintainer's guide to CONTRIBUTING.md, and templates for issues and PRs.

cc @ibrasho @darkowlzz @carolynvs 😄

What should your reviewer look out for in this PR?

Typos, things that don't make sense, wordiness, etc. in the guide.

Which issue(s) does this PR fix?

Helps with #629, but doesn't quite fix it.


-->

### What version of Go (`go version`) and `dep` are you using?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we don't have dep version, let's ask people to run git describe --tags.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's time we gave #209 some love? 😁
git describe --tags has to be run within dep directory within GOPATH to report dep version and that isn't clear for many people and they might end up reporting their git repo tags (or worse report an error if their git repo doesn't have tags).

### What should your reviewer look out for in this PR?

### Which issue(s) does this PR fix?

Copy link
Collaborator

Choose a reason for hiding this comment

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

### Are there any parts that you are need help/answers/clarification?

@@ -0,0 +1,12 @@
### What does this do / why do we need it?
Copy link
Collaborator

Choose a reason for hiding this comment

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

### PR Title
Put the word WIP in the title if you are submitting this PR and are continuing to work on it. That way we'll know it's not 100% finished.

CONTRIBUTING.md Outdated
@@ -1,6 +1,6 @@
# Contributing to Dep

Dep is an open source project.
`dep` is an open source project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to change other instances of Dep to dep in this doc?, like on line 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, worth doing


### General guidelines

* _Be kind, respectful, and inclusive_. Really live [that CoC](https://github.com/golang/dep/blob/master/CODE_OF_CONDUCT.md). We've developed a reputation as one of the most welcoming and supportive project environments in the Go community, and we want to keep that up!
Copy link
Collaborator

Choose a reason for hiding this comment

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

💖

CONTRIBUTING.md Outdated
* We use [Zenhub](https://www.zenhub.com) to manage the queue, in addition to what we do with labels.
* Pipelines, and [the board](https://github.com/golang/dep#boards) are one thing we try to utilize
* Marking dependencies/blockers is also quite useful where appropriate; please do that.
* We use epics and milestones in roughly the same way (because OSS projects don't have real sprints). Epics should be duplicated as milestones; if there's a main epic issue, it should contain a checklist of the relevant issues to complete it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to know! I'll make sure we have that for the init importers.


### Issue management

* We use [Zenhub](https://www.zenhub.com) to manage the queue, in addition to what we do with labels.
Copy link
Collaborator

@carolynvs carolynvs Jun 6, 2017

Choose a reason for hiding this comment

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

* Zenhub Pipelines
  * `New Issues`: When someone creates a new issue, it goes here first. Keep an eye out for issues that fall into your area. Add labels to them, and if it's something we should do, put it in the `Backlog` pipeline. If you aren't sure, throw it in the `Icebox`. It helps to sort this pipeline by date.
  * `Icebox`: Issues that we aren't immediately closing but aren't really ready to be prioritized and started on. It's not a wontfix bucket, but a "not sure if we should/can fix right now" bucket.
  * `Backlog`: Issues that we know we want to tackle. You can drag/drop up and down to prioritize issues.
  * Hopefully the rest are self explanatory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to just push that change directly to this branch 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added!

CONTRIBUTING.md Outdated

* Try to encourage, and make, smaller pull requests.
* [No is temporary. Yes is forever](https://blog.jessfraz.com/post/the-art-of-closing/).
* Long-running feature branches should generally be avoided. Discuss it with other maintainers first.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unclear on what constitutes a long running feature branch but I know I'm a repeat offender. 😊 I usually only realize after the fact and am not always sure when to cry uncle and how to fix it though.

Or are you referring to something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry, no, I didn't mean long-running PRs (though those pose a similar problem). I meant an actual branch in the dep repo that we merge multiple PRs into, with the intent of merging it all into master later.

CONTRIBUTING.md Outdated
* [No is temporary. Yes is forever](https://blog.jessfraz.com/post/the-art-of-closing/).
* Long-running feature branches should generally be avoided. Discuss it with other maintainers first.
* Unless it's trivial, don't merge your own PRs - ask another maintainer.
* Commits should follow [Tim Pope's rules](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html).
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you feel about contributors submitting PRs with magic commands like "Fixes #X" in the commit message?

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 doesn't bother me - we can always edit their OP and change it if it's inaccurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

My preferences is to give contributors as much ownership and control as we reasonably can. In cases like this, where the effort required of maintainers to manage that gifted power is minimal, IMO it's entirely worth it.

CONTRIBUTING.md Outdated
* Does the first post in the PR contain "Fixes #..." text for any issues it resolves?
* Are any necessary follow-up issues _already_ posted, prior to merging?
* Does this change entail the updating of any docs?
* For docs kept in the repo, e.g. FAQ.md, docs changes _must_ be submitted as part of the same PR.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.


Paste the output of the commands you ran in here, making sure to pass -v for maximum context.

Including the ouptut of `dep hash-inputs` may also be helpful to include here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/ouptut/output/

CONTRIBUTING.md Outdated
### Issue management

* We use [Zenhub](https://www.zenhub.com) to manage the queue, in addition to what we do with labels.
* Pipelines, and [the board](https://github.com/golang/dep#boards) are one thing we try to utilize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing punctuation at line end.

@darkowlzz
Copy link
Collaborator

LGTM 👍

As we are discussing how we would be maintaining dep well with multiple maintainers, how about using some tools to help us in doing so.
I came across zappr. It offers a bunch of tools to help maintainers. Among them, I like Approval check, Pull request labels check and Pull request tasks check.

Maybe we can look into it in the future, but I feel such tools, especially approval check, would help us in avoiding manually looking for approval comments and sometimes, even making mistakes.

Thoughts?

@sdboyer
Copy link
Member Author

sdboyer commented Jun 6, 2017

If zappr does what it claims, then I'm definitely 👍 . Commit message rules and approval enforcement would both be beneficial.

@sdboyer sdboyer merged commit 9e747df into golang:master Jun 7, 2017
@sdboyer sdboyer changed the title Maintainer's guide, et. all Maintainer's guide, et al. Jul 26, 2017
This pull request was closed.
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.

5 participants