Update contributing.md with new merge policy #2789

jackzampolin · 2018-11-12T22:24:05Z

Per discussion about SDK merge policy

codecov · 2018-11-12T22:31:51Z

Codecov Report

Merging #2789 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2789      +/-   ##
==========================================
- Coverage    56.71%   56.7%   -0.02%     
==========================================
  Files          131     131              
  Lines         8530    8536       +6     
==========================================
+ Hits          4838    4840       +2     
- Misses        3359    3363       +4     
  Partials       333     333

CONTRIBUTING.md

zramsay · 2018-11-12T23:45:57Z

CONTRIBUTING.md

+* If your change relates to the core SDK (baseapp, store, ...), please update the docs/gaia folder, the docs/examples folder and possibly the docs/spec folder.
+* If your changes relate specifically to the gaia application (not including modules), please modify the docs/gaia folder.
+* If your changes relate to a module, please update the module's spec in docs/spec. If the module is used by gaia and/or basecoin, you might also need to modify docs/gaia and/or docs/examples.
+* If your changes relate to the core of the CLI or Light-client (not specifically to module's CLI/Rest), please modify the docs/clients folder.


is this section meant to be duplicate of the update docs/DOCS_README ?

Yes. We might just want to point that file to this spot in the CONTRIBUTING.md doc

CONTRIBUTING.md

Co-Authored-By: jackzampolin <[email protected]>

rigelrozanski · 2018-11-13T00:00:45Z

CONTRIBUTING.md

+All PRs require 2 Reviews before merge. When reviewing PRs please use the following annotations:
+
+- `LGTM` without an explicit approval means that the changes look good, but you haven't pulled down the code, run tests locally and thoroughly reviewed it.
+- `Approval` through the GH UI means that you understand the code, documentation/spec is updated in the right places, you have pulled down and tested the code locally.


K so I guess the only funny issue, is when you perform a "request changes" the only way to override that is via an approve right? If that's the case then maybe you want to "approve" something in the github sense to indicate that you're satisfied with the changes made, however this doesn't mean you've reviewed it deeply.... For that reason I think we may still need DeepAck

I think the answer here is only Approve if you have fully reviewed the changes. If you Request Changes that would mean you need to fully review to Approve which seems like a good policy (if you know enough to request changes on the PR, then you should also be able to review it fully). Thoughts?

Yup that sounds fine, so I guess if you are making only "surface level" reviews, you can submit your review as a "comment" instead of a "request changes"

want to incorporate that idea into this section?

you can also dismiss your review after you request changes.

cwgoes

Several more points suggested.

cwgoes · 2018-11-13T12:40:14Z

CONTRIBUTING.md

+
+### Process for Reviewing PRs
+
+All PRs require 2 Reviews before merge. When reviewing PRs please use the following review explanations:


reviews (no capitalization)

Strange. I had made a suggestion and now it's gone.

cwgoes · 2018-11-13T12:41:46Z

CONTRIBUTING.md

+All PRs require 2 Reviews before merge. When reviewing PRs please use the following review explanations:
+
+- `LGTM` without an explicit approval means that the changes look good, but you haven't pulled down the code, run tests locally and thoroughly reviewed it.
+- `Approval` through the GH UI means that you understand the code, documentation/spec is updated in the right places, you have pulled down and tested the code locally.


Can we add a few more points for reviewers:

You must also think through anything which ought to be included but is not

You must think through whether any added code could be partially combined (DRYed) with existing code

You must think through any potential security issues or incentive-compatibility flaws introduced by the changes

cwgoes · 2018-11-13T13:10:04Z

Should we have an additional peerACK for peer-reviewed (as in, on a call or in person)?

jackzampolin · 2018-11-13T17:28:18Z

I like that idea @cwgoes I'll go ahead and add that. Maybe Peer Reviewed with an approval instead?

rigelrozanski · 2018-11-14T18:25:33Z

CONTRIBUTING.md

+
+### Process for reviewing PRs
+
+All PRs require 2 Reviews before merge. When reviewing PRs please use the following review explanations:


Thinking about this more - do we really want two reviews for all PRs - for instance I think

doc change PRs is sufficient with one approval.

variable name change PRs I think are also sufficient with a single approval.

Good point @rigelrozanski. I'll incorporate this.

jaekwon · 2018-11-14T20:22:43Z

CONTRIBUTING.md

+- `Approval` through the GH UI means that you understand the code, documentation/spec is updated in the right places, you have pulled down and tested the code locally. In addition:
+  - You must also think through anything which ought to be included but is not
+  - You must think through whether any added code could be partially combined (DRYed) with existing code
+  - You must think through any potential security issues or incentive-compatibility flaws introduced by the changes


Also:

naming must be consistent with the rest of the codebase

code must belong in a reasonable location, considering dependency structures (e.g. not importing testing modules in production code, or including example code modules in production code).

if you approve of the PR, you are responsible for fixing any of the issues mentioned here and more.

Adding these.

CONTRIBUTING.md

Co-Authored-By: jackzampolin <[email protected]>

Comments from this review have been addressed and incorporated.

@rigelrozanski

* Back to 50 initially bonded * Add query deposits cmds * Update test * Update PENDING.md * Back to on-operation=false, update PENDING.md * Remove unnecessary change, clarify amt in sim_test.go * Cleanup, additional item in PENDING.md * Update PENDING.md Co-Authored-By: alessio <[email protected]> * Update client/keys/utils.go Co-Authored-By: alessio <[email protected]> * update depositer addr * Address @rigelrozanski comments * Make linter happy * Update PENDING.md * Remove shorthand * Make "multi" const * Store last total power as sdk.Int, not sdk.Dec * Merge PR cosmos#2553: Renamed msg.Name() and msg.Type() to msg.Type() and msg.Route() * Fix stupid mistake * s/number/weight/ * LastValidatorPower is also an Int * Merge PR cosmos#2462: Add swagger-ui for gov, stake and slashing * 'make format' * Move PENDING to CHANGELOG * Linkify changelog * Fix db write perm * Additional cleanup * Remove logs from prior debugging * Merge PR cosmos#2599 from cosmos/jae/dist_refactor * Rename Pool -> DelRewards; PoolCommission -> ValCommision * FeePool.Pool -> FeePool.ValPool * WithdrawalHeight->DelPoolWithdrawalHeight * OnValidatorBeginUnbonding * Caught the bug's tail * Update vi.FeePoolWithdrawalHeight upon bonding * Fix staking slashUnbondingDelegation bug; fixes simulator failure cosmos#9 * Merge PR cosmos#2597: Add distribution accum invariants * PENDING.md => CHANGELOG.md * Manually linkify * Manually fix some links * Docs fixes in progress while running through the release process * More docs fixes * '--voter' is no longer required * Rectify validator setup documentation * Merge PR cosmos#2596: Cmds for validator unbondings and redelegations * Make simulation use a transition matrix for block size This enables simulating periods of high load, and periods of low to no load. (low load because future ops will still terminate in that time frame) * address bez's comments * fix flags in docs, closes cosmos#2530 * Merge PR cosmos#2616: Block redelegations to the same validator * Merge PR cosmos#2623: Speedup simulator by switching to goleveldb Due to requiring app.Commit() at the moment, golevel db is significantly faster than a memdb * fix block offsets in printing simulation block number * Merge PR cosmos#2644: Simulation: Print last block when there is an error There was an off by one error in the log printing function previously * Merge PR cosmos#2642: Add todo diagrams * Various sign command improvements - Exit with error if the user is attempting to sign with a key whose address is not among those who are expected to sign the transaction. - Add --print-signature-only to output only the generated signature. * Check sanity of signatures and report errors when run with --print-sigs * Improve errors reporting * Improve online docs * Refresh PENDING.md * Find better name for --print-signature-only * Fix integration tests * Validate --name * Fix integration tests * s/--print-sigs/--validate-signatures/ * s/--sig-only/--signature-only/ * Docs updated * Update PENDING.md * Rename append, it's go builtin * Set success = false when it fails * Apply suggestions from bez * Nest switches * Fix rebase * Document what --validate-signatures does * perform minor doc and function cleanup * move typedef * Merge PR cosmos#2614: Configurable Bech32 prefix for SDK users * Merge PR cosmos#2643: AppendTag function usage error. append elements do not work * simulation: Make validator choice use validator set This also had to change the default seed, since with the previous one it actually got into a state where there were no validators left bonded, lol. This also changes Unbond msgs from failing with almost 100% probability to now only failing with 33% probability. Thus more of the state machine is getting tested! * Update changelog * Merge PR cosmos#2657: Fix config.js * Merge PR cosmos#2589: Update Vesting Spec * Merge PR cosmos#2656: Revert read-only leveldb database * Revert read-only leveldb database Waiting on a fix for syndtr/goleveldb#240. * Update client/keys/utils.go * Include DNS alt name in certificate Closes: cosmos#2664 * Gaialite signal handling is broken, repair it * Merge PR cosmos#2665: simulation: Remove header from Invariant This got introduced recently, but wasn't actually needed, hence the reversion * Merge PR cosmos#2653: Add benchmark for get and set account * Fix test * Refactor TrapSignal * Fix lint * enforcing @jaekwon mergemaster * added querier redelegation * added validatorDelegations querier endpoint * LCD and CLI * cli fixes * removed redelegation stuff * address other comments * rebased * addressed comments * Make the simulator create the new comission rate sensibly * Update to TM v0.26.0 - Part I (cosmos#2679) * Update to TM v0.26.0 * Bez/tm0.26 update pt 2 redux (cosmos#2684) * Update to TM v0.26.0 * Update TODOs * Proof and verification updates * Fix linting * Fix key path creation * Temporarily fix tendermint revision to make tests pass * Fix merge conflict bug; Update PENDING * New genesis workflow (cosmos#2602) New genesis workflow: * `gaiad init` is now used to generate an empty `genesis.json`. * Genesis accounts need to be populated manually before running `gaiad collect-gentxs`. * This should support starfish too, see cosmos#2615 for more info. * Closes: cosmos#2596 cosmos#2615 * Validate validator address and address against respective account ex ante * Fix local testnet failures * New genesis tests * Run make format * Add --pubkey flag * gaiad collect-gentxs takes no args * Simulation improvements (logging fix, random genesis parameters) (cosmos#2617) * Print out initial update on every block * Randomize simulation parameters * Randomize initial liveness weightings * Randomize genesis parameters * fixed power store invariant * IterateValidatorsBonded -> IterateBondedValidatorsByPower * WriteValidators uses IterateLastValidators rather than IterateBondedValidatorsByPower * fixed democoin interface Closes cosmos#2556 Closes cosmos#2396 Via cosmos#2671: closes cosmos#2669 closes cosmos#2670 closes cosmos#2620 Offshoot issues: cosmos#2618 cosmos#2619 cosmos#2620 cosmos#2661 * Fix simulation bugs; Incorprates cosmos#2676 from Sunny (cosmos#2677) * Fix simulation bugs; Incorprates cosmos#2676 from Sunny * Address review feedback; Update PENDING * 'make format' * Revert "enforcing @jaekwon mergemaster" This reverts commit 15c2093. * Update x/stake/client/rest/query.go Co-Authored-By: sunnya97 <[email protected]> * addressed fede's comment * Switch gov proposal-queues to use iterators (cosmos#2638) * switched gov proposals queue to use iterators * update gov spec * update proposal.Equal * Amino api change * switched proposalID to uint64 * renamed Gov Procedures to Params * s/ActiveProposalQueueProposalKey/KeyActiveProposalQueueProposal/g * numLatestProposals -> Limit * fixed staking invariant breakage because of gov deposits * Send deposits to DepositedCoinsAccAddr or BurnedDepositCoinsAccAddr * Add general merkle absence proof (also for empty substores) (cosmos#2685) * Fix coins.IsLT() impl (cosmos#2686) * Fix coins.IsLT() impl * Fix coin.IsLT() impl * Coins.IsLT -> Coins.IsAllLT etc * Update testnet to use canonical genesis time (cosmos#2692) * Update testnet to use canonical genesis time * Fix linting in genesis test * Do not allow nil values to be set in CacheKVStore (cosmos#2708) * Do not allow nil values to be set in CacheKVStore * Makefile OS compatibility update * Merge PR cosmos#2714: Add commission data to MsgCreateValidator signature bytes * PENDING => CHANGELOG * Linkify changelog * Cleanup bank keeper * whitespacing * rand utile ... * moving stuff around a bit, trying to get rid of types * reorganize more * rename ambig naming of queueOperations * minimizing indentation * fix some duplicate to get passing * Address style comments * Reorganize CLI command structure. Fixes cosmos#2575 * Fix missing flags issue * Address linting issues * Fix gobash CLI testing * Fix typo * Cross-compiling get_tools Makefile added * operations functions * assertAllInvarients changes, Operation reorg * mock tendermint * util cleanup * event stats object, more general cleanup * compiling * pending * Removed comment from Makefile as per bez's request * val comments * Address PR comments * Update cmd/gaia/cmd/gaiacli/main.go Co-Authored-By: jackzampolin <[email protected]> * PENDING * Fix state export/import, add to CI (cosmos#2690) * Update slashing import/export * More slashing.WriteGenesis * Add test import/export to CI * Store equality comparison. * Fix validator bond intra-tx counter * Set timeslices for unbonding validators * WriteGenesis => ExportGenesis * Delete validators from unbonding queue when re-bonded * Hook for validator deletion, fix staking genesis tests * Merge 0.26.0 back to develop (cosmos#2718) * PENDING => CHANGELOG * Linkify changelog * Merge PR cosmos#2716: Temporarily disable gaia lite insecure mode * TODO: need to update CHANGELOG w/ import-export PR cosmos#2690 * Update CHANGELOG/PENDING for straggling PR cosmos#2690 * Add small utility to add account to genesis.json after gaiad init * Update CHANGELOG.md * s/WriteGenesisFile/ExportGenesisFile/ * Update PENDING.md * Add --chain-id to testnet command * Address remaining comments from cosmos#2690 * Update PENDING.md * add back in PeriodicInvariant * Linter fix * Fix TimeoutCommit (cosmos#2743) * Fix TimeoutCommit to 5 seconds instead of whatever it was before which was too short. * Gaia-9000: Update to TM 0.26.1-rc2 (cosmos#2753) * Update to tm 0.26.1-rc2 to fix prometheus issue and node disconnect issue. * Gaia-9000: Update to TM 0.26.1-rc3 -- pex SeedMode fix * fix typo I think it might be a spelling mistake * Slight distribution spec cleanup * More cleanup * use defer * Use correct Bech32 prefix for show-address command (cosmos#2746) * Use consensus address bech32 prefix * Update show-address CLI description * Generate random moniker when missing * Update moniker prefix * Require moniker instead of generating a random one * update to tendermint v0.26.1 * Fix test coverage * Correctly set return code * Fix date to be cross platform * Merge PR cosmos#2752: Don't hardcode bondable denom * R4R: Fix unbonding command flow (cosmos#2727) * Fix required flag * Fix redelegation command * Add pending entry * update swagger.yaml * use newQuery...Params * Link to issue * Fix DiffKVStore * Address PR review * Working on stake import/export * Only apply validator set updates on initial genesis * Clarify comment * Fix failing test * add back in CLI command after rebase * Fix CLI tests * update to amino 0.14.1 * pending * R4R: Query Gov Params (cosmos#2576) * gov query params * Merge PR cosmos#2744: Fix Makefile targets dependencies * Fix Makefile targets dependencies * Remove unnecessary build deps from install targets * Create a rule for each tool * Don't dep test_lint on tools * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <[email protected]> * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <[email protected]> * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <[email protected]> * Update overview.md * Documentation Structure Change and Cleanup (cosmos#2808) * Update docs/sdk/clients.md * organize ADR directory like tendermint * docs: move spec-proposals into spec/ * remove lotion, moved to website repo * move getting-started to cosmos-hub, and voyager to website * docs: move lite/ into clients/lite/ * move introduction/ content to website repo * move resources/ content to website repo * mv sdk/clients.md to clients/clients.md * mv validators to cosmos-hub/validators * move deprecated sdk/ content to _attic * sdk/modules.md is duplicate with modules/README.md * consolidate remianing sdk/ files into a single sdk.md * move examples/ to docs/examples/ * mv docs/cosmos-hub to docs/gaia * Add keys/accounts section to localnet docs * Bring back banner (cosmos#2814) * Build docs in CircleCI (cosmos#2810) * error checking the API call * added docs build trigger to circleci job * Update contributing.md with new merge policy (cosmos#2789) * Update contribuiting.md with new merge policy * deleted obsolete file (cosmos#2817)

Update contribuiting.md with new merge policy

51a91cd

jackzampolin requested review from cwgoes, ebuchman and rigelrozanski as code owners November 12, 2018 22:24

jackzampolin mentioned this pull request Nov 12, 2018

R4R: Docs cleanup #2783

Merged

Add note about documentation updates

7d13028

alexanderbez reviewed Nov 12, 2018

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

zramsay reviewed Nov 12, 2018

View reviewed changes

rigelrozanski reviewed Nov 12, 2018

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

alexanderbez and others added 2 commits November 12, 2018 15:58

Update CONTRIBUTING.md

72142a6

Co-Authored-By: jackzampolin <[email protected]>

Update CONTRIBUTING.md

4e52ad6

Co-Authored-By: jackzampolin <[email protected]>

rigelrozanski reviewed Nov 13, 2018

View reviewed changes

jackzampolin added 2 commits November 12, 2018 16:29

Address PR comments

3804e0a

Address PR comments

303f264

cwgoes previously requested changes Nov 13, 2018

View reviewed changes

cwgoes changed the title ~~Update contribuiting.md with new merge policy~~ Update contributing.md with new merge policy Nov 13, 2018

jackzampolin added 2 commits November 14, 2018 10:16

Add peer review comment (#2811)

3b29591

Address PR comments

c76fb6c

rigelrozanski reviewed Nov 14, 2018

View reviewed changes

jaekwon reviewed Nov 14, 2018

View reviewed changes

Address PR comments

11ce509

rigelrozanski approved these changes Nov 14, 2018

View reviewed changes

gamarin2 suggested changes Nov 14, 2018

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

gamarin2 and others added 2 commits November 14, 2018 12:46

Update CONTRIBUTING.md

bbf8cca

Co-Authored-By: jackzampolin <[email protected]>

Address @GaMarin's comments

20b2e07

gamarin2 approved these changes Nov 14, 2018

View reviewed changes

Merge branch 'develop' into jack/contrib

dca52ff

jackzampolin merged commit 0453992 into develop Nov 14, 2018

jackzampolin deleted the jack/contrib branch November 14, 2018 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update contributing.md with new merge policy #2789

Update contributing.md with new merge policy #2789

jackzampolin commented Nov 12, 2018

codecov bot commented Nov 12, 2018 •

edited

Loading

zramsay Nov 12, 2018

jackzampolin Nov 12, 2018

rigelrozanski Nov 13, 2018

jackzampolin Nov 13, 2018

rigelrozanski Nov 13, 2018

jackzampolin Nov 13, 2018

ValarDragon Nov 13, 2018

cwgoes left a comment

cwgoes Nov 13, 2018

alexanderbez Nov 13, 2018

cwgoes Nov 13, 2018

cwgoes commented Nov 13, 2018 •

edited

Loading

jackzampolin commented Nov 13, 2018

rigelrozanski Nov 14, 2018 •

edited

Loading

jackzampolin Nov 14, 2018

jaekwon Nov 14, 2018

jackzampolin Nov 14, 2018


		### Process for Reviewing PRs

		All PRs require 2 Reviews before merge. When reviewing PRs please use the following review explanations:

Update contributing.md with new merge policy #2789

Update contributing.md with new merge policy #2789

Conversation

jackzampolin commented Nov 12, 2018

codecov bot commented Nov 12, 2018 • edited Loading

Codecov Report

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

cwgoes left a 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

cwgoes commented Nov 13, 2018 • edited Loading

jackzampolin commented Nov 13, 2018

rigelrozanski Nov 14, 2018 • edited Loading

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

codecov bot commented Nov 12, 2018 •

edited

Loading

cwgoes commented Nov 13, 2018 •

edited

Loading

rigelrozanski Nov 14, 2018 •

edited

Loading