-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add Contributing Guidelines #1422
Conversation
004e678
to
e1fbc24
Compare
CONTRIBUTING.md
Outdated
|
||
Please follow standard github best practices: fork the repo, branch from the tip of develop, make some commits, and submit a pull request to develop. See the [open issues](https://github.com/cosmos/cosmos-sdk/issues) for things we need help with! | ||
|
||
Please make sure to use `gofmt` before every commit - the easiest way to do this is have your editor run it for you upon saving a file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add make test_lint
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
changelog int
e1fbc24
to
696a308
Compare
|
||
Thank you for considering making contributions to Cosmos-SDK and related repositories! Start by taking a look at this [coding repo](https://github.com/tendermint/coding) for overall information on repository workflow and standards. | ||
|
||
Please follow standard github best practices: fork the repo, branch from the tip of develop, make some commits, and submit a pull request to develop. See the [open issues](https://github.com/cosmos/cosmos-sdk/issues) for things we need help with! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link to issues tagged with "help-wanted" (and do a better job at tagging issues "help-wanted")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
|
||
We use [dep](https://github.com/golang/dep) to manage dependencies. | ||
|
||
That said, the master branch of every Cosmos repository should just build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we promise this can we test it automatically somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't circle ci have to run make install
in order to run the cli tests, so this already would be checked as long as master is passing tests. (Which it always should)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure we can not sure we need to though?
CONTRIBUTING.md
Outdated
- the latest state of development is on `develop` | ||
- `develop` must never fail `make test` | ||
- no --force onto `develop` (except when reverting a broken commit, which should seldom happen) | ||
- create a development branch either on github.com/cosmos/cosmos-sdk, or your fork (using `git add origin`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git remote add origin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think so - addressed
CONTRIBUTING.md
Outdated
|
||
### Pull Merge Procedure: | ||
- ensure pull branch is rebased on develop | ||
- run `make test` to ensure that all tests pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test test_cli
, at least at the moment, they can also just run make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks added
CONTRIBUTING.md
Outdated
- ensure pull branch is rebased on develop | ||
- run `make test` to ensure that all tests pass | ||
- merge pull request | ||
- the `unstable` branch may be used to aggregate pull merges before testing once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting; do we have an unstable
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we used too - I'm deleting for now
CONTRIBUTING.md
Outdated
|
||
### Release Procedure: | ||
- start on `develop` | ||
- run integration tests (see `test_integrations` in Makefile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely do not have a test_integrations
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
Codecov Report
@@ Coverage Diff @@
## develop #1422 +/- ##
=======================================
Coverage 63% 63%
=======================================
Files 118 118
Lines 6573 6573
=======================================
Hits 4141 4141
Misses 2161 2161
Partials 271 271 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, looks like CI is failing (strangely) though
Also let's add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few remarks, but otherwise 👍 .
- start on `develop` | ||
- prepare changelog/release issue | ||
- bump versions | ||
- push to release-vX.X.X to run CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to elaborate on versioning conventions (especially for hotfixes)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added link to https://semver.org
- before submitting a pull request, begin `git rebase` on top of `develop` | ||
|
||
### Pull Merge Procedure: | ||
- ensure pull branch is rebased on develop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
develop
* 👍
CONTRIBUTING.md
Outdated
|
||
Libraries need not follow the model strictly, but would be wise to | ||
|
||
### Development Procedure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's implicit that the contributed work adheres to our linting standards correct (e.g. formatting, vet, imports, etc...)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a reference to make test_lint
.
…dk into rigel/contributing_guide
c9ee621
to
c4f9d87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
* Merge pull request #1422: Add Contributing Guidelines * cwgoes comments
Adapted from Tendermint figured we want this with all the external contributors coming around
N/A: