swarm: add pull request template #1715

acud · 2019-08-31T04:47:31Z

Adds a PR template to the repo for the welfare of all our contributors and reviewers alike

pull_request_template.md

nolash · 2019-09-02T07:35:50Z

pull_request_template.md

+1. Open PRs preemptively (i.e. whenever you start working on it), especially for the case of large tasks
+2. Create PRs with a healthy amount of description. A PR without a description will not be approved
+3. PR names should adhere to the following standard: "(affected package(s)): description". examples: "storage: fix panic on hasherstore put", "network, storage, p2p: integrate capabilities api". if many (many) packages are affected, use "all" instead of individual names
+3. Mark the PR with a `ready for review` label once it is. If a second(or more) round of reviews is required, remove the `ready for review` label and add a `ready for another review` label


use "in progress" inbetween

TBH i find the in progress label redundant and never use it. if the package is not marked ready for review it means it is in progress and we should not review it. not sure why we need a label for every permutation of the universe

If it is first "ready for review" and then needs some more work I change it to "in progress" while it's not ready for review anymore. I think this protocol makes it more clear when to re-review PRs.

as @nolash says it is in order for a second reviewer to judge if submitter works on fixes atm.

nolash · 2019-09-02T07:36:06Z

pull_request_template.md

+2. Create PRs with a healthy amount of description. A PR without a description will not be approved
+3. PR names should adhere to the following standard: "(affected package(s)): description". examples: "storage: fix panic on hasherstore put", "network, storage, p2p: integrate capabilities api". if many (many) packages are affected, use "all" instead of individual names
+3. Mark the PR with a `ready for review` label once it is. If a second(or more) round of reviews is required, remove the `ready for review` label and add a `ready for another review` label
+4. Never(!) force-push a PR once reviews have started! This has the unfortunate side-effect of resolving/outdating previous comments over the PR


so rebase only in the end?

ideally yes. and ideally you shouldn't need to rebase at all. i'm willing to reconcile with adding "rebasing while the pr is under review is to be done only if requested by reviewers"

this is more complicated. if the PR review is a longer process, it is super important to keep it rebased, since it can bring up incompatibilities etc.
Another reason not to keep review process too long and PRs to be small.
@janos you usually do a merge what do u think is the best way?

This point is fine as a warning, even I doubt that it can be always applied, but at least it would minimize force pushing. As we cannot restore old history after force pushing, I do not see what we can do when that happens. It has a bit of conflict with point number 1. With preemptive PRs, we are encouraging early comments, which means that this point would be activated very early.

I think that rebasing and force pushing breaks history for other clones, especially when conflicts need to be resolved afterwards repeatedly. Having a merge commit allows better automatic conflict resolution by not loosing commits and history. But, I have nothing against force pushing as it provides cleaner history. It is completely ok to do force push for me, as it is a usual operation on swarm codebase. I would prefer forbidding force pushing completely, as we are squashing commits at the end, but that would be too drastic.

Should we encourage development in parallell temporary files where possible? That way we have less issues with rebase.

My idea is to avoid force pushing as much as possible, that is why I added the reservations that the reviewer can ask to do so once the review process has started.

Rebasing brings a plethora of problems ranging from regressions due to human error while rebasing to github quirks that resolve comments and other branches that break while rebasing. These thing happen to us all the time and I would like to reduce the number of points of possible failures. We shouldn't rebase unless another branch contains a changeset that is needed in the current one we're working on. If master changes in the meanwhile when we work on the PR - it does not infer that a rebase is in need. If our CI runs a merge build and not a push build the incompatibilities should pop up and be shown on the CI build.

I agree that this point somehow is in conflict with point 1, but the idea is to give guidelines to contributors while allowing core members a bit more flexibility according to fluctuating needs. There's no silver bullet here but we can at least minimize error by new and external contributors.

@nolash you are welcome to PR or start a document on development best practices. I really don't wanna make a PR template into a gocode best practices knowledge-base

@acud my point is not a go-specific one. Conflicts would happen in any kind of source. By keeping separate temporary files, rebase is even less likely. This is also aligned with your announced goal:

My idea is to avoid force pushing as much as possible

nolash · 2019-09-02T07:36:21Z

pull_request_template.md

+3. Mark the PR with a `ready for review` label once it is. If a second(or more) round of reviews is required, remove the `ready for review` label and add a `ready for another review` label
+4. Never(!) force-push a PR once reviews have started! This has the unfortunate side-effect of resolving/outdating previous comments over the PR
+5. It is up to the reviewer to resolve a comment
+6. A comment does not account as a blocker to merging a PR unless explicitly mentioned in the comment: `BLOCKER: .....`


nolash · 2019-09-02T07:38:43Z

pull_request_template.md

+6. A comment does not account as a blocker to merging a PR unless explicitly mentioned in the comment: `BLOCKER: .....`
+7. When reviewing - please make the comment on a line that would constitute a change when addressed - this will resolve the comments automatically once addressed
+8. Once you address a review comment with a code change - no need to comment on it with "done"/"ok"/:+1: - this generates redundant email notifications (to those who have them enabled) and once you push git will automatically resolve/outdate the comment and the reviewer could then mark the comment as resolved without any further taxation of reading even more comments on the PR
+9. We are all here to ship working software with a continuous attention to technical excellence and good design (https://agilemanifesto.org/principles.html); not to make each other's lives miserable. Thus, we ask you to take PR reviews as a process of reflection rather than a fencing duel(!)


I'd like to add something like "in difference of opinion focus on the arguments." and perhaps "matters of pure taste should be the discretion of thte author"

can you please write the complete suggested bulletpoint?

nolash · 2019-09-02T07:39:37Z

pull_request_template.md

+9. We are all here to ship working software with a continuous attention to technical excellence and good design (https://agilemanifesto.org/principles.html); not to make each other's lives miserable. Thus, we ask you to take PR reviews as a process of reflection rather than a fencing duel(!)
+10. Please reference related issues that should be closed with the most favorite keyword of your choice - https://github.com/blog/1506-closing-issues-via-pull-requests
+11. Always use the `Squash` method of merging the PR to the branch
+12. Always tidy up the commit message when using squash - having a very long commit message does not help anyone. Summerize the work done in bullet points and only then merge


how about "including trivial intermediate commit messages" - sometimes maybe it's not a dumb idea to have a bit of a log.

TBH in the context of big pull requests, intermediate commit messages are redundant because the underlying diff is very difficult to compare anyway. in the case of small PRs intermediate commits are trivial. i'm willing to reconcile with adding a small request to include intermediate commit message only in the case that they are very important

big pull requests

But there shouldn't be any, right? ;)

Yep. especially on planet nolash

nonsense

Overall looks good to me.

zelig · 2019-09-04T04:24:11Z

I would add:

before setting the ready for review label, make sure all checks passed. To try the ci locally, use go run ./build/ci.go lint, go run ./build/ci.go test.
for merging the PR must be reviewed and approved by at least two others and approved by all parties that submitted a review earlier even if there are 2 other approvals
you do not need to create an issue on a PR you submit (early or complete, cos you would not anyway) but if there exists one, reference it with the appropriate magic keywords, e.g., fixes #1501
consider submitting changes in other packages that are dependecies of a PR in separate PRs
assign to yourself or whoever you delegate
make sure the work is shown on a board by choosing the project
use labels
- review status: in progress, ready for review, ready for another review
- it is prudent to use 'in progress' before the PR is ready, but not needed
- it is imperative to use 'ready for review'
- in subsequent iterations, be brave and put back in progress, and ready for review
PRs are an excellent interface to discuss changes, so sometimes you open PRs that are not meant to be merged. Use the 'do not merge' label for PR that are
- experimental (e.g., DO NOT MERGE Independent services in Swarm #1676) or
- demonstrative (Unified hasher methods in swarm #427)
- supplemental (alternative implementation, e.g, pss: Improve pressure backstop queue handling #1680)
put on the board and groom the board regularly, esp blocker

nolash · 2019-09-04T06:57:26Z

for merging the PR must be reviewed and approved by at least two others and approved by all parties that submitted a review earlier even if there are 2 other approvals

@zelig @acud we should also add a reminder that before merge smoke tests on cluster should have been run and passed.

nolash · 2019-09-04T07:00:08Z

@acud is it a conscious choice that you have omitted encouragement to keep the PR deltas small?

nolash · 2019-09-04T07:05:21Z

9b. If there is strong difference of opinion, please double check that you have understood all proposed arguments in the process of discussion. Unresolved differences are resolved by simple majority of author and the two reviewers. Matters of taste are at the discretion of the author.

How about this? @acud

nolash · 2019-09-04T07:45:09Z

we should also add a reminder that before merge smoke tests on cluster should have been run and passed.

...though in case of wholly external contributors this is not possible, I guess.

acud · 2019-09-04T09:25:51Z

@zelig @nolash I have nothing against your propositions but may I remind you that these guidelines should appear in the following (tiny) textbox:

Not sure how much text you wanna have in there but I feel that if we are not concise with a readable checklist people will just not read what's in the box. We are better off with having this in some wiki page/documentation page.

swarm: add pull request template

1541823

acud requested review from janos, nolash, nonsense and zelig August 31, 2019 04:47

nolash reviewed Sep 2, 2019

View reviewed changes

address pr comments

9157c9c

janos approved these changes Sep 2, 2019

View reviewed changes

acud self-assigned this Sep 3, 2019

nonsense approved these changes Sep 3, 2019

View reviewed changes

acud closed this Sep 4, 2019

acud reopened this Oct 20, 2019

address pr comments

34a4c66

acud merged commit 99fc4dd into master Oct 20, 2019

acud deleted the pr-template branch October 20, 2019 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm: add pull request template #1715

swarm: add pull request template #1715

acud commented Aug 31, 2019

nolash Sep 2, 2019

acud Sep 2, 2019

nolash Sep 2, 2019

zelig Sep 4, 2019

nolash Sep 2, 2019

acud Sep 2, 2019

zelig Sep 3, 2019

janos Sep 3, 2019

nolash Sep 3, 2019

acud Sep 3, 2019

nolash Sep 4, 2019

nolash Sep 2, 2019

nolash Sep 2, 2019

acud Sep 2, 2019

nolash Sep 2, 2019

acud Sep 2, 2019

nolash Sep 2, 2019

acud Sep 3, 2019

nonsense left a comment

zelig commented Sep 4, 2019

nolash commented Sep 4, 2019 •

edited

Loading

nolash commented Sep 4, 2019

nolash commented Sep 4, 2019 •

edited

Loading

nolash commented Sep 4, 2019

acud commented Sep 4, 2019 •

edited

Loading

swarm: add pull request template #1715

swarm: add pull request template #1715

Conversation

acud commented Aug 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense left a comment

Choose a reason for hiding this comment

zelig commented Sep 4, 2019

nolash commented Sep 4, 2019 • edited Loading

nolash commented Sep 4, 2019

nolash commented Sep 4, 2019 • edited Loading

nolash commented Sep 4, 2019

acud commented Sep 4, 2019 • edited Loading

nolash commented Sep 4, 2019 •

edited

Loading

nolash commented Sep 4, 2019 •

edited

Loading

acud commented Sep 4, 2019 •

edited

Loading