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

PR Policy for DApps of POA Network #3

Open
igorbarinov opened this issue Dec 30, 2017 · 3 comments
Open

PR Policy for DApps of POA Network #3

igorbarinov opened this issue Dec 30, 2017 · 3 comments

Comments

@igorbarinov
Copy link
Member

igorbarinov commented Dec 30, 2017

Title

  Title: Generic PR Policy for DApps of POA Network
  Layer: Project Management

Abstract

In this RFC we propose a PR policy for contributors and developers of DApps in POA Network.

Rationale

At the moment, the PR policy is implemented in ICO Wizard DApp. It helps manage processes with the distributed team. In this proposal, we generalize and summarize PR police

Specification

Each PR should have:

  • Mandatory Description
    a human-readable description of changes
    a human-readable description of the purpose of the PR
  • Recommended What is it: (Fix), (Feature), or (Refactor) in Title, e.g., "(Fix) price of 1 token in Wei > 18 decimals"
  • Recommended Developers: Each completed PR should be updated in the Wiki Documentation.
  • Recommended Each PR should have one commit message and therefore should only contain one specific fix or feature. Otherwise, multiple PRs should be made
  • Optional Any additional concerns or comments

Security

  • All DApps should have protected branches for production dapps e.g.Core
    • protected branch should have "Protect this branch", "Require pull request reviews before merging", "Include administrators" options enabled
@afck
Copy link

afck commented Jun 19, 2018

I'm not sure about the third bullet point; there are many PRs that don't really fit one of those categories. Even with e.g. the Leaf guidelines, my experience was that often changes wouldn't fit in one of their eight categories.

Would it make sense to also make this "Recommended" instead, and/or allow using other keywords?

@igorbarinov
Copy link
Member Author

igorbarinov commented Jun 20, 2018

good idea!
Changed to Recommended .. anyway no one rejected PRs for missing this bullet point requirement 😬

@afck
Copy link

afck commented Jun 26, 2018

Another topic we could include in such a document is which reviewer(s) to pick and when to merge.

  • Should external contributors try to pick a reviewer or wait for the team to assign one?
  • Should team members assign several reviewers, or is it better to pick one (and maybe cc others)?
  • In a PR with several reviewers, can you merge with one approval or should you wait until they all approved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants