Update the contributing guidelines #2611

floriank · 2015-02-23T10:16:05Z

Goal

In the light of some of the latest pull requests it became apparent hat we lack a single source of truth when it comes to the guidelines for contributing to OpenProject.

After some reviewing and looking into the history, I updated the guidelines for contributing to OpenProject to reflect current practices.

Notes

Information has been pulled together from different source, mainly the Guidelines from the OpenProject wiki (which is no longer available to me), comments made by @linki in #1362 and the discussion in #2594, as well as discussion with @ulferts on current practices in handling PRs.

There also was a discussion with @myabc last week on using CLAHub for contributors to the repository, to which these guidelines have no reflection of. I'd be happy on discussing this topic, as there is some need of that in my opinion.

Ticket

https://community.openproject.org/work_packages/18827

Update the contributing guidelines to provide a single source of truth for contributions from the community. Signed-off-by: Florian Kraft <f.kraft@finn.de>

also, use a different phrasing for the grace period.

TeatroIO · 2015-02-23T10:16:06Z

I've prepared a stage to preview changes. Open stage or view logs.

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare · 2015-02-23T14:46:30Z

CONTRIBUTING.md

@@ -1,7 +1,50 @@
 OpenProject is an open source project and we encourage you to help us out. We'd be happy if you do one of these things:


I think you should also reword this intro or keep the list of things that someone might want to do to help us (create tickets, be active in forum, etc.).

tl; dr: Your write-up below does not match this paragraph

Funnily enough, I had a different version which got lost in my git-fu.

I'd reword it to something along the lines of:

"If you want to help us, please read the following guidelines to contributing:"

Optionally we could reword the individual headlines to be more "pro-active", like "Creating tickets in OP", "Creating new features", "Creating hotfixes", etc.

The timestamp is superfluous, as the information for contributing should not expire over time. [ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare · 2015-02-23T14:55:07Z

CONTRIBUTING.md

+Ideally, there is a work package for every issue that is tackled by a contributor - the list can be found [here](https://community.openproject.org/projects/openproject/work_packages). Work packages can be created there on-demand if necessary.
+
+## Development flow
+For contributing source code, please follow the Git Workflow below:


We should probably add some wording to indicate, that the rules below apply to community contributions.

While the OpenProject core developers should also try to stick to these rules, they might deviate from them, e.g. not fork the repo, but have a branch on the main repository.

Proposal:

For external contributions [...] please follow the Git Workflow below:

Signed-off-by: Florian Kraft <f.kraft@finn.de>

myabc · 2015-02-23T15:01:14Z

CONTRIBUTING.md

+## Development flow
+For contributing source code, please follow the Git Workflow below:
+
+- Fork Openproject on GitHub


OpenProject

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare · 2015-02-23T15:04:25Z

CONTRIBUTING.md

+## Important notices
+
+- Please add tests to your code to verify functionality, especially if it is a new feature. Please also run these tests locally.
+- Please create pull requests against the current `dev` branch. Hotfixes and Bugfixes should be created against the appropiate `release/*` branch.


I think the information around non-dev PRs is incomplete:

How do I find out which branch is the right one?

Which kinds of PRs are allowed there (e.g. no new features)

Should I make two PRs in such a case? (one against release/x and one against dev?)

the answer is no... but I can't see it ^^

You have to help me out here. My understanding after a couple of weeks:

Features/gem updates go against dev

Hotfixes go against latest release/*

Bugfixes go into dev

Backports (bugfixes/hotfixes) go into release/*

[skip ci] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare · 2015-02-23T15:10:57Z

We require a CLA to be signed, which you do not currently mention anywhere.
I'll ask Birthe whether CLAHub is an option for us (especially legally). Otherwise the correct way should be something like sending a letter o.O

edit: One thing that I find worrying about CLAHub is that the last activity there was two years ago...

myabc · 2015-02-23T15:15:30Z

Otherwise the correct way should be something like sending a letter o.O

Electronically-signed CLA's are used by a lot of big companies now. IANAL, but I'm pretty sure electronic signatures are also valid in our jurisdiction.

If we're required to have paper CLA's – sent my post – we might as well just write f*** you. we don't take contributions 😜

NobodysNightmare · 2015-02-23T15:18:01Z

We are in germany here... I am not sure what qualifies as electronic signature either ^^

And what does IANAL mean?

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare · 2015-02-23T15:27:26Z

After actually reading the CLA I realized, that the CLA states how it should be transferred:

Please complete the agreement and send it together with a detached gpg signature via email to cla@openproject.org

This should be the way then... until we decide internally, that CLAHub is also fine...

floriank · 2015-02-23T16:00:15Z

I am not a lawyer. IANAL.

NobodysNightmare · 2015-02-24T08:05:02Z

What about Rubocop/Hound? Do I have to care about code style and coding conventions?

floriank · 2015-02-24T08:49:37Z

I would add a section for style on this for new contributions, i.e. lines you touch.
This would include a note on hound as well as using rubocop -a on the files to be commited.

Also, include commentary on style guidelines and pull request reviewing. Signed-off-by: Florian Kraft <f.kraft@finn.de>

This adds a paragraph for the CLA requirement for external contributors. Signed-off-by: Florian Kraft <f.kraft@finn.de>

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

Azure7111 · 2015-02-25T08:20:57Z

CONTRIBUTING.md

+
+```
+rubcocop -a ./path/to/file
+```


This will obviously not fix all of the stylecop offenses. We might want to specifically give the hint that it is useful to check manually nevertheless.

However, I'm not sure if that would be out of scope for this document and should be noted somewhere else.

Well, the goal imho should be to have it all in one place, otherwise we will have the Problem of multiple documents floating around having to be cared for as well.

We could rephrase that paragraph into a suggestion that this can be done to improve the style of one's own code.

At the end of the day my idea of abiding code style guidelines would simply be to not introduce new offenses (no matter what kind) and maybe even go the extra mile to remove some others as well. I usually like to remove the other offenses in the files I touched in a PR. However this could really put our rubocop configuration to the test, which might not be as desirable.

I would not explicitly suggest that for the community (going the extra mile), but keep that as our mission...

This will not stop brave members of the community to fix the style nevertheless, but I think this is nothing we need to anticipate inour guidelines.

jonasheinrich · 2015-02-26T15:03:57Z

CONTRIBUTING.md

+
+Please add tests to your code to verify functionality, especially if it is a new feature.
+
+Pull requests will be verified by TravisCI as well, but please run them locally as well. We have a lot of pull requests coming in and it takes some time to run the complete suite for each one.


Maybe we should say here that it would be best if the tests were green when run locally. So there is a chance that Travis needs to test this PR only once.

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

just make sure they are green. [ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

myabc · 2015-02-26T15:30:55Z

@floriank to reduce the amount of work Travis has to do, you can add [ci skip] to your commit messages.

floriank · 2015-02-26T15:33:12Z

@myabc I think I added that - but in a more demanding way:

Please also use [ci skip] in your commit message to suppress builds which are not necessary (e.g. after fixing a typo in the README).

myabc · 2015-02-26T15:33:55Z

@floriank oh yes. / gitignore me

floriank · 2015-02-26T15:47:22Z

echo "@myabc" >> .gitignore

Azure7111 · 2015-02-27T15:07:54Z

Should contributions to plugins follow the same guidelines?

floriank · 2015-02-27T15:10:10Z

Yeah, I guess so.

kgalli · 2015-02-27T15:12:05Z

I strongly recommend they do!

floriank · 2015-02-27T15:18:34Z

Question is - how would we redirect the users - just place a link to this CONTRIBUTING.md in the CONTRIBUTING.md in the plugin repos?

floriank · 2015-02-27T17:41:22Z

I will keep this PR open till Monday - after that it will be merged.

myabc · 2015-02-28T17:43:01Z

CONTRIBUTING.md

+
+### Etiquette and communication
+
+Lastly, be nice and respectful to each other. We are working hard to make OpenProject the best project management software there is and we are grateful for each contribution. 


@floriank Bundler now adds a Code of Conduct with its gem generator.

https://github.com/bundler/bundler/blob/master/lib/bundler/templates/newgem/CODE_OF_CONDUCT.md.tt

http://contributor-covenant.org/

Angular are also using it.

Might be something to consider.

Thanks, I'll have a look at it after workout 💪

myabc · 2015-02-28T17:53:36Z

👍

myabc · 2015-02-28T18:00:51Z

Two things that just occurred to me:

how (or will) Teatro instances work with this workflow?
ditto for Rubocop.

Sorry for late feedback.

floriank · 2015-02-28T18:10:18Z

Teatro and Hound should be available for all PRs - am I missing something? Rubocop can also be executed locally.

Features which would be developed by multiple developers can be created as a pull request set as work in progress - alternatively there is still the option to have a branch in the main repo, if desirable. the goal here should be to reduce the overall number.

myabc · 2015-03-01T08:56:39Z

Teatro and Hound should be available for all PRs - am I missing something? Rubocop can also be executed locally.

Right, of course. I was missing something.

ulferts · 2015-03-02T10:28:55Z

CONTRIBUTING.md

+
+Please add tests to your code to verify functionality, especially if it is a new feature. 
+
+Pull requests will be verified by TravisCI as well, but please run them locally as well and make sure there are green before creating your pull request. We have a lot of pull requests coming in and it takes some time to run the complete suite for each one.


typo:

and make sure there are green

->

and make sure they are green

ulferts · 2015-03-02T10:42:39Z

I think it's good enough. There will be stuff we might want to improve but I rather have some guidelines now that we can improve later on.

We can issue a PR changing and following the guidelines at the same time. WHOA.

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

floriank · 2015-03-02T10:58:27Z

This is it folks. i am merging this. Thanks for all your contributions.

Here is a picture of my cat:

Update the contributing guidelines

* parallelize build matrix * use phantomjs 2.0 * use postgres 9.3 * run custom scripts * whiteliste main branches

Florian Kraft and others added 2 commits February 23, 2015 10:56

updates the contributing guidelines

764d9ff

Update the contributing guidelines to provide a single source of truth for contributions from the community. Signed-off-by: Florian Kraft <f.kraft@finn.de>

emphasize grace period

d56e9bc

also, use a different phrasing for the grace period.

add a note on connection to work packages

5731bdb

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare reviewed Feb 23, 2015
View reviewed changes

remove the timestamp

fa11ee3

The timestamp is superfluous, as the information for contributing should not expire over time. [ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare reviewed Feb 23, 2015
View reviewed changes

increase grace period, add not on WIP label

1f69f4f

Signed-off-by: Florian Kraft <f.kraft@finn.de>

myabc reviewed Feb 23, 2015
View reviewed changes

reword the first paragraph

ca88991

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

NobodysNightmare reviewed Feb 23, 2015
View reviewed changes

remove typo - It's called OpenProject

fb56e92

[skip ci] Signed-off-by: Florian Kraft <f.kraft@finn.de>

Florian Kraft added 2 commits February 23, 2015 16:18

explicilty state the WIP label name

23129e1

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

remove strange typo

7bc15ae

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

Florian Kraft added 3 commits February 24, 2015 10:31

split up the important notes

1a3912f

Also, include commentary on style guidelines and pull request reviewing. Signed-off-by: Florian Kraft <f.kraft@finn.de>

add paragraph for CLA

aac40fe

This adds a paragraph for the CLA requirement for external contributors. Signed-off-by: Florian Kraft <f.kraft@finn.de>

add a note for force pushing and cancelling builds

db7b444

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

Azure7111 reviewed Feb 25, 2015
View reviewed changes

jonasheinrich reviewed Feb 26, 2015
View reviewed changes

Florian Kraft added 3 commits February 26, 2015 16:07

correct typos, clarify exceptions

4902697

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

specify gitter channel

cd02afc

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

emphasize on running tests

801ae37

just make sure they are green. [ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

floriank mentioned this pull request Feb 27, 2015

Remove TravisCI branch restrictions according to new contribution guidelines #2638

Merged

myabc reviewed Feb 28, 2015
View reviewed changes

ulferts reviewed Mar 2, 2015
View reviewed changes

Florian Kraft added 3 commits March 2, 2015 11:47

update the branching model to be more specific

37ee9e7

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

add the Contributors Code of Conduct

3c5bea6

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

Move the Code of Conduct to it's own file

579183d

[ci skip] Signed-off-by: Florian Kraft <f.kraft@finn.de>

floriank added a commit that referenced this pull request Mar 2, 2015

Merge pull request #2611 from floriank/fix/update-contributing

af0bb7f

Update the contributing guidelines

floriank merged commit af0bb7f into opf:dev Mar 2, 2015

myabc mentioned this pull request Apr 27, 2015

17056 Set up Sauce Labs for Protractor tests #2245

Closed

3 tasks

marutosi referenced this pull request in basiszwo/openproject Nov 10, 2015

Update travis configuration

a60893d

* parallelize build matrix * use phantomjs 2.0 * use postgres 9.3 * run custom scripts * whiteliste main branches

		@@ -1,7 +1,50 @@
		OpenProject is an open source project and we encourage you to help us out. We'd be happy if you do one of these things:


		Please add tests to your code to verify functionality, especially if it is a new feature.

		Pull requests will be verified by TravisCI as well, but please run them locally as well. We have a lot of pull requests coming in and it takes some time to run the complete suite for each one.


		### Etiquette and communication

		Lastly, be nice and respectful to each other. We are working hard to make OpenProject the best project management software there is and we are grateful for each contribution.

Update the contributing guidelines #2611

Update the contributing guidelines #2611

Conversation

floriank commented Feb 23, 2015

Goal

Notes

Ticket

TeatroIO commented Feb 23, 2015

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

NobodysNightmare commented Feb 23, 2015

myabc commented Feb 23, 2015

NobodysNightmare commented Feb 23, 2015

NobodysNightmare commented Feb 23, 2015

floriank commented Feb 23, 2015

NobodysNightmare commented Feb 24, 2015

floriank commented Feb 24, 2015

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

myabc commented Feb 26, 2015

floriank commented Feb 26, 2015

myabc commented Feb 26, 2015

floriank commented Feb 26, 2015

Azure7111 commented Feb 27, 2015

floriank commented Feb 27, 2015

kgalli commented Feb 27, 2015

floriank commented Feb 27, 2015

floriank commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myabc commented Feb 28, 2015

myabc commented Feb 28, 2015

floriank commented Feb 28, 2015

myabc commented Mar 1, 2015

Choose a reason for hiding this comment

ulferts commented Mar 2, 2015

floriank commented Mar 2, 2015