Add contribution workflow #98
Conversation
|
/cc @joyent/node-coreteam. |
|
wow that sounds complicated. i would be -0, giving i am wearing my apache On Thu, Mar 26, 2015 at 2:05 AM, Julien Gilli [email protected]
|
|
@robertkowalski Thank you for your feedback! I'm interested in reading more about what precisely seems complicated to you. Can you please comment within the change itself? |
|
Sure! First off all I really like the document as I see and feel that your intention was to make it easier for maintainers. I know that maintaining projects often comes with mixed feelings. The main argument I have is that the document describes a lot of "default processes". So far i have worked in environments that value "people over process". Given I would follow all these rules described in the doc (forgive me, I am German, we love rules) I honestly would never merge a PR at all, or maybe once every week. I probably would merge some, but a lot of my time would just be tagging them and marking them and doing all the "paperwork" until they get merged. I think the workflow document is cool and a good starter for many issues in a corporate environment, but I see so many ad-hoc stuff happening in Open Source where I feel this might not be the best fit for landing a patch from the community for the community <3 Does that somehow makes sense? Do you have questions? Happy to help! |
|
Thank you for the more detailed feedback @robertkowalski 👍 The document in this PR describes two strict rules:
The rest of the document describes workflows and tools that are optional. Thus, I would like to focus on the two mandatory points mentioned above and try to understand the concerns you have about them first.
One of them is that it allows us to communicate what is included or is going to be included in upcoming releases between core team members, but also with the rest of the community (external contributors, users, etc.). Another advantage is that it allows all collaborators of the project to focus on what is important for the next upcoming releases: instead of navigating through hundreds of issues and PRs, anyone can just click on a link and find what's in the scope of the next releases, which is always a much smaller subset that is much easier to navigate through. Regarding 2), Node.js is a runtime that works on many different platforms and in the past we've had problems with changes that had not been properly tested on all of these platforms, especially for stable releases. When that happens, core contributors need to fix the problem and run tests manually on all supported platforms. This is a very time consuming process. Moreover, at that point, bugs need to be fixed in a timely manner, which puts the contributor(s) investigating these bugs in a situation that is not comfortable. In this context ,having tests run before any change is landed allows contributors:
Basically, it requires less work from every contributor while giving them more confidence in the changes that are included in every release. Last but not least, these guidelines have been written to be used within the joyent/node repository. They wouldn't apply to any other repository, at least for now. I would like to read more about the specific concerns that you may have regarding these two points. Thank you again! |
|
|
||
| To start testing any pull-request, simply follow the following step: | ||
|
|
||
| 1. Point your broser to [the node-accept-pull-request Jenkins |
|
@misterdjules that makes sense, thank you for explaining - that the other points are optional was somehow not obvious for me. I am also a huge fan of CI and running the testsuite before each merge. I also think the tagging is a good idea. I was just thinking in the first place that all steps are required, also the ones for the community (e.g. the nominating). Thank you for taking the time and explaining it again to me, really appreciated! |
|
|
||
| For a non core team member to nominate an issue or a pull-request for a | ||
| milestone, add a comment of the following form: | ||
| ``` |
There was a problem hiding this comment.
maybe:
Nominating a milestone for non core team members
Non core members can nominate an issue or a pull-request for a milestone if they want.
To nominate an issue, just add a comment of the following form:
|
left some feedback based on my misconception and your explanation |
|
@robertkowalski Thank you for taking the time to provide some very valuable feedback and improving this document! |
|
I just want to add that as somebody who will have to run the accept job I fully support having to run the tests before a pull request is merged, we'll just have to watch that it does not slow things down as it does take a while to execute. If we see that happening (ie queued up jobs, which I've not seen yet) we might have to look at a method that would let multiple PRs be validated together. |
|
+1 for this proposal, especially for the two required points. Some of the other points might or might not get adopted as described, but I think it's useful to have them illustrated especially for new collaborators / contributors. I have never seen a project suffer from too much onboarding documentation ;) |
|
@robertkowalski Updated the PR according to your feedback, thanks again! |
|
LGTM |
misterdjules
force-pushed
the
add-contribution-workflow
branch
from
3e5abaa to
c41c1a5
Compare
PR: #98 PR-URL: #98 Reviewed-By: Robert Kowalski <[email protected]>
|
Thank @robertkowalski and everyone who provided feedback, landed in f931cce. |
This pull request is to make the improvements about our workflow we already talked about more broadly available.
However. this is not the same version as the draft that I had sent originally.
This PR only includes what we have used so far in that document. The rest of the changes proposed in that original draft will be added to our workflow incrementally, and the corresponding section of the website will be updated at the same time.