Issue 95: Adding first version of development process documentation. #106
Conversation
|
Please verify the committer name, email, and GitHub username association are all correct and match CLA records. |
|
@twilmes — you seem to have the same issue as @blacknred0 did in PR #96, namely that GitHub is missing the metadata for your username in your commit, so the automated tool cannot tell who submitted this PR. @blacknred0 submitted a ticket to GitHub support and they fixed it; please do the same. Once GitHub fixes it, @janusgraph-bot will flip the CLA bit for you automatically. |
docs/development.txt
Outdated
|
|
||
| Though not required, it is strongly recommended that contributors limit the amount of development work they | ||
| perform before their change is accepted. In the instance where the change is not approved or significant updates to | ||
| the proposal are made, we wish to minimize the amount of rework required. |
There was a problem hiding this comment.
I'm not sure I understand the impetus behind this paragraph. What would be an example?
There was a problem hiding this comment.
I think this is superfluous upon rereading so I'm taking it out. My intent was to discourage folks from spending a ton of time on development before seeking consensus on whether or not a change is a good idea or not but I think the process makes that clear already.
docs/development.txt
Outdated
| For features that involve only one developer, developers will work in their own JanusGraph forks, managing their | ||
| own branches, and submitting pull requests when ready. Feature branches in the main JanusGraph repository will be | ||
| created when more than one developer collaborates on a feature. | ||
|
|
There was a problem hiding this comment.
Do we see a problem with developers working together on individual forks outside of the main JanusGraph repository, as opposed to feature branches?
Maybe just have a simple process along the lines of "If multiple developers wish to collaborate on a feature, they may request a feature branch be created."
Another question that comes to mind is what if those working on a feature branch are not committers? Do we assign some sort of "shepherd" who can merge pull requests for that branch?
There was a problem hiding this comment.
Perhaps this means that the owner of a feature (=feature branch) should be a committer?
docs/development.txt
Outdated
| pull request process. In instances where the committer deems the full process | ||
| unnecessary, commit-then-review (CTR) may be employed. This should be used infrequently and only for extremely trivial | ||
| changes. Changes of this sort may include: | ||
|
|
There was a problem hiding this comment.
For CTR, we should have some wording around vetoing (-1) a commit, so if a committer objects to something already committed it can be rolled back.
| * Documentation typo or small documentation addition | ||
| * Addition of a new test case | ||
|
|
||
| Any commit that is made following CTR shall include the fact that it is a CTR in the commit comments. |
There was a problem hiding this comment.
Do we want to encourage some sort of notification in these cases? "Hey I just committed this, if anyone objects, let me know and I'll revert."
There was a problem hiding this comment.
GitHub provides automated notifications: https://help.github.com/articles/managing-notifications-for-pushes-to-a-repository/
For example, we can create a list with the firehose of all commits / merges, e.g., janusgraph-commits@, and folks who want the firehose can subscribe to this. I've seen this done with the commit-then-review workflow in the LLVM project, it worked well.
I can set this up.
There was a problem hiding this comment.
This is now done; please see https://groups.google.com/forum/#!forum/janusgraph-commits
docs/development.txt
Outdated
|
|
||
| * Three +1 votes are required merge a pull request | ||
| * One or more -1 votes will veto the pull request until the noted issue(s) are addressed and the pull request | ||
| is resubmitted |
There was a problem hiding this comment.
maybe change to "and the -1 vote is withdrawn."
It would also be good to any -1 vote must include an explanation, and that a -1 without an explanation is not valid.
docs/development.txt
Outdated
| * Committers may vote on their own pull requests | ||
| * Pull requests that are limited to one component require at least one of the +1 votes to come from the relevant | ||
| TSC member. If TSC members are unable to review after 2 weeks, the pull request may be merged | ||
| after three +1 votes |
There was a problem hiding this comment.
Do we need to codify a rule that requires a relevant TSC member to vote, and otherwise imposes a 2 week delay?
It might be simpler to just require +1s from committers knowing they will do the right thing, like giving the TSC member time or a nudge to review.
Also sometimes when projects are under heavy pull request load, it can be difficult to get 3 +1s. Some projects do something like "one +1 from a committer who is not the author of the pull request."
There was a problem hiding this comment.
I'll remove the TSC requirement. I think that's too stringent to start and we always add it in later if need be.
docs/development.txt
Outdated
| consensus is reached, decide on a date to start the release code freeze. Allow for one week between the start of the | ||
| code freeze and the beginning of the release vote. During that week, a release manager shall create release | ||
| artifacts. Committers will be given seven days to review and vote on the release artifacts. Three +1 votes are required for a release to happen. | ||
| If one or more committers casts a -1 vote, the release will be halted. |
There was a problem hiding this comment.
In Apache projects, release votes can't be vetoed, they require at least 3 +1 votes and more +1s than -1s. Just pointing that out in case we want to follow suit.
docs/development.txt
Outdated
| https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] proposing the new release and requesting feedback on what should be included in the release. After | ||
| consensus is reached, decide on a date to start the release code freeze. Allow for one week between the start of the | ||
| code freeze and the beginning of the release vote. During that week, a release manager shall create release | ||
| artifacts. Committers will be given seven days to review and vote on the release artifacts. Three +1 votes are required for a release to happen. |
There was a problem hiding this comment.
Is a code freeze necessary? Couldn't we just create a release branch that forms the base for the release (i.e. so work on master can continue)?
We may also want to shorten the vote time (Apache uses min. 72hrs.). As currently worded it looks like there would be a minimum turnaround of two weeks for a release. What if, for example we need to rush out a security patch?
There was a problem hiding this comment.
I agree with the release branch approach, lets unblock ourselves.
There was a problem hiding this comment.
We need a break-glass mechanism for releases. If we are affected by a CVE, we would like to act even sooner. Usually CVE can be mitigated by updating dependency versions, so in that case the mechanics of creating a release would only be limited by TinkerPop test time? Currently I understand this to be 24 hours.
docs/development.txt
Outdated
| https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] proposing the new release and requesting feedback on what should be included in the release. After | ||
| consensus is reached, decide on a date to start the release code freeze. Allow for one week between the start of the | ||
| code freeze and the beginning of the release vote. During that week, a release manager shall create release | ||
| artifacts. Committers will be given seven days to review and vote on the release artifacts. Three +1 votes are required for a release to happen. |
There was a problem hiding this comment.
I agree with the release branch approach, lets unblock ourselves.
docs/development.txt
Outdated
| For features that involve only one developer, developers will work in their own JanusGraph forks, managing their | ||
| own branches, and submitting pull requests when ready. Feature branches in the main JanusGraph repository will be | ||
| created when more than one developer collaborates on a feature. | ||
|
|
There was a problem hiding this comment.
Perhaps this means that the owner of a feature (=feature branch) should be a committer?
docs/development.txt
Outdated
| Pull Requests | ||
| ------------- | ||
|
|
||
| Most changes to JanusGraph must be reviewed before they are committed (RTC). This workflow is performed using Github's |
docs/development.txt
Outdated
| https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] proposing the new release and requesting feedback on what should be included in the release. After | ||
| consensus is reached, decide on a date to start the release code freeze. Allow for one week between the start of the | ||
| code freeze and the beginning of the release vote. During that week, a release manager shall create release | ||
| artifacts. Committers will be given seven days to review and vote on the release artifacts. Three +1 votes are required for a release to happen. |
There was a problem hiding this comment.
We need a break-glass mechanism for releases. If we are affected by a CVE, we would like to act even sooner. Usually CVE can be mitigated by updating dependency versions, so in that case the mechanics of creating a release would only be limited by TinkerPop test time? Currently I understand this to be 24 hours.
janusgraph-bot
added
cla: yes
twilmes
force-pushed
the
issue_95_developer_docs
branch
from
d204f89
to
4e4cabf
Compare
docs/development.txt
Outdated
| For features that involve only one developer, developers will work in their own JanusGraph forks, managing their | ||
| own branches, and submitting pull requests when ready. If multiple developers wish to collaborate on a feature, | ||
| they may request that a feature branch be created in JanusGraph repository. If the developers are not committers, a | ||
| JanusGraph committer will be assigned as the shepard for that branch. The shepard will be responsible for merging |
docs/development.txt
Outdated
| @@ -0,0 +1,84 @@ | |||
| [[development-process]] | |||
| JanusGraph Development Process | |||
| ============================== | |||
There was a problem hiding this comment.
Please use Asciidoc best practices for section titles, as described in their docs.
Specifically, this line should be:
= JanusGraph Development Process
and other section headings should use the number of leading = equal to their header level.
The current approach of using different types of characters on a line is complex (need to remember which character means which heading level) and error-prone (must equal in length to the heading, or else it breaks. This is the type of thing I had to deal with during the global s/Titan/Janusgraph/ cleanup; see this commit for more details.
I am working on a change to convert all existing headers to the Asciidoc-recommended approach, so it would be nice to not add any more of these anachronistic section titles.
There was a problem hiding this comment.
Good call. I'll also move back to sentence per line.
docs/development.txt
Outdated
|
|
||
| Users wishing to contribute to JanusGraph should fork the https://github.com/janusgraph/janusgraph[JanusGraph] | ||
| repository and then submit pull requests using the Github pull request process. Please include the relevant issue | ||
| number in the title of your pull request. A vote (-1, 0, +1) is required before a pull request may be merged. |
There was a problem hiding this comment.
Instead of +1 / 0 / -1, can we just use the GitHub approval flow? This makes the approvals stand out, and it's easy to count them: they show up as checkmarks in the right-hand side vs. having to scan for +1 in the message stream.
Only committers / maintainers will have the ability to approve a PR, which makes it easy to tell which ones are binding vs. non-binding +1s.
docs/development.txt
Outdated
| Non-committers are free, and encouraged to vote. Their votes will be non-binding, but can be taken into consideration | ||
| and are still very valuable community input. The following rules apply to the voting process. | ||
|
|
||
| * Three +1 votes are required merge a pull request |
There was a problem hiding this comment.
Is this standard practice for ASF projects to require three +1s for every commit? Is this not too many? I ask because a requirement of 3 reviewers with +1 may slow down the review / submit process for PRs. Thoughts?
There was a problem hiding this comment.
Have the same feeling that 3 sounds a little too much.
How about this? Two +1 or approvals from other than the author.
In this way, committer author does not needs to explicitly cast +1.
There was a problem hiding this comment.
There isn't a standard across ASF projects so I was just going off what we do on TinkerPop in this case. Two +1 approvals from other than the author sounds good to me.
docs/development.txt
Outdated
| RTC process should be followed. | ||
|
|
||
| Users wishing to contribute to JanusGraph should fork the https://github.com/janusgraph/janusgraph[JanusGraph] | ||
| repository and then submit pull requests using the Github pull request process. Please include the relevant issue |
There was a problem hiding this comment.
Could you clarify the relationship between Issue and PR? Every PR needs to be associated with an Issue? I am for it.
Could we include some recommendations for the commits going into the pull request? i.e. the commit message, organize the commits.
There was a problem hiding this comment.
In the PR, include a statement of how the Pull Request has been tested. e.g. unit test, new test, or other testing.
docs/development.txt
Outdated
| * Call a vote to approve the release on https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] | ||
|
|
||
| Committers will be given 72 hours to review and vote on the release artifacts. A minimum of three +1 votes and more | ||
| +1's than -1's are required for a release to be approved. |
There was a problem hiding this comment.
Could you clarify this? Is this more relaxed than the rule for a Pull Request? Should we just do the same -1 policy as for PR?
There was a problem hiding this comment.
@ptgoetz I know this was just a suggestion, you okay with me switching this back to a -1 vetoing a release? I'm on the fence with this one because in many cases, I would imagine if anyone felt there was a serious enough issue to warrant a veto, we'd probably be in agreement, but maybe not...
There was a problem hiding this comment.
@twilmes Yes that's fine. The way it usually works on Apache projects is that the release manager takes a -1 seriously and usually cancels the release vote to address it. In my experience as a release manager I've never let a release proceed if there were binding -1 votes.
|
Pushed an update, I'll squash commits after we reach consensus and are ready to merge. |
docs/development.txt
Outdated
| However, for larger bodies of work and significant updates and additions, the following process shall be followed. | ||
| Significant work may include, but is not limited to: | ||
|
|
||
| * A major new feature or subproject eg. a new storage adapter |
docs/development.txt
Outdated
| * Start a DISCUSS thread on the https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] list where the proposed change may be discussed by committers | ||
| and other community members | ||
| * When the proposer feels it appropriate, a VOTE shall be called | ||
| * Three +1 votes are required for the change to be accepted. |
There was a problem hiding this comment.
Did you want to change this to be "Two +1 votes are required..."?
docs/development.txt
Outdated
| == Branching | ||
|
|
||
| For features that involve only one developer, developers will work in their own JanusGraph forks, managing their own branches, and submitting pull requests when ready. If multiple developers wish to collaborate on a feature, | ||
| they may request that a feature branch be created in JanusGraph repository. If the developers are not committers, a JanusGraph committer will be assigned as the sheperd for that branch. The sheperd will be responsible for merging pull requests to that branch. |
docs/development.txt
Outdated
| == Pull Requests | ||
|
|
||
| Most changes to JanusGraph must be reviewed before they are committed (RTC - review-then-commit). | ||
| This workflow is performed using Github's pull request process. In instances where the committer deems the full process unnecessary, commit-then-review (CTR) may be employed. |
| * Documentation typo or small documentation addition | ||
| * Addition of a new test case | ||
|
|
||
| Any commit that is made following CTR shall include the fact that it is a CTR in the commit comments. |
There was a problem hiding this comment.
GitHub provides automated notifications: https://help.github.com/articles/managing-notifications-for-pushes-to-a-repository/
For example, we can create a list with the firehose of all commits / merges, e.g., janusgraph-commits@, and folks who want the firehose can subscribe to this. I've seen this done with the commit-then-review workflow in the LLVM project, it worked well.
I can set this up.
docs/development.txt
Outdated
| * Prepare the release artifacts | ||
| * Call a vote to approve the release on https://groups.google.com/forum/#!forum/janusgraph-dev[janusgraph-dev] | ||
|
|
||
| Committers will be given 72 hours to review and vote on the release artifacts. |
There was a problem hiding this comment.
This paragraph is written as a bulleted list (each sentence starts on a new line), but that's not how it will be rendered in HTML. If you'd like it to become a bulleted list, please make it so. Otherwise, please reflow the paragraph so that it reads as it will be rendered.
There was a problem hiding this comment.
Hm, not sure if I understand. When I build the docs this renders as a bulleted list as I expected.
There was a problem hiding this comment.
I'm not referring to "Prepare" and "Call", I'm referring to "Committers will be given ..." and following sentences. That also renders as a bulleted list?
docs/development.txt
Outdated
|
|
||
| * A major new feature or subproject eg. a new storage adapter | ||
| * Incrementing the version of a core dependency such as a Apache TinkerPop | ||
| * Addition or deprecation of a public API |
There was a problem hiding this comment.
Bringing in a major dependency.
docs/development.txt
Outdated
| Their review will be non-binding, but can be taken into consideration and are still very valuable community input. | ||
| The following rules apply to the voting process. | ||
|
|
||
| * Two committer approvals are required merge a pull request |
docs/development.txt
Outdated
|
|
||
| === Branch Naming Conventions | ||
|
|
||
| All branch names shall be prepended with `Issue_#_`. |
There was a problem hiding this comment.
For example, if there was an issue 114 to add Tupl support to JanusGraph, and people needed to collaborate on this branch, then it could be named Issue_114_Tupl. This naming convention only applies to hosted branches created for collaboration, not to branches in forks created for normal pull requests.
There was a problem hiding this comment.
Updated to note that this only applies to the main JanusGraph repo.
|
|
||
| Users wishing to contribute to JanusGraph should fork the https://github.com/janusgraph/janusgraph[JanusGraph] repository and then submit pull requests using the GitHub pull request process. | ||
| Every pull request must be associated with an existing issue. | ||
| Be sure to include the relevant issue number in the title of your pull request and a description of what test suites were run to validate the pull request. |
There was a problem hiding this comment.
CI will run tests for you (ideally). Eventually, we should probably ask for contributors to note any special testing steps they took.
There was a problem hiding this comment.
Agreed. I think we should leave this in for now and then when CI is in good shape, we can update.
twilmes
force-pushed
the
issue_95_developer_docs
branch
from
fa10910
to
c249fda
Compare
|
I made a few updates based upon @amcp feedback. At this point there has been good review of this so I cleaned up the commits. Are we ready to merge? |
|
I'm ok with the changes, how about @mbrukman ? |
|
@mbrukman you okay with leaving the CTR text as is and then when we get the janusgraph-commits firehose setup, I'll make an update to remove the requirement to notify the dev list? If so, I think we can go ahead and merge and close this issue out. |
|
@twilmes — https://groups.google.com/forum/#!forum/janusgraph-commits is now being populated. |
| * Documentation typo or small documentation addition | ||
| * Addition of a new test case | ||
|
|
||
| Any commit that is made following CTR shall include the fact that it is a CTR in the commit comments. |
There was a problem hiding this comment.
This is now done; please see https://groups.google.com/forum/#!forum/janusgraph-commits
|
Thanks for creating the list @mbrukman. I'll add the link in and get this merged. |
twilmes
force-pushed
the
issue_95_developer_docs
branch
from
c249fda
to
9955f4f
Compare
Issue 95: Adding first version of development process documentation.
Issue 95: Adding first version of development process documentation.
Here is a first cut at developer process docs. The release policy section is pretty sparse but I think we can expand this when we go through our first release process. I imagine we'll also add explicit release steps and directions at that point also. I'm not wed to any of these particular policies but I think they're a good start. Specifically I'm curious if folks think we should allow CTR in limited cases like I've defined here.