Skip to content
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 release and reviewing tutorial to contributing guide #53

Merged
merged 18 commits into from
Jan 3, 2023
37 changes: 37 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Copyright 2020-2022 Alibaba Group Holding Limited.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

name: Release

on:
push:
tags:
- "v*"


# Automatically create a GitHub Release, with release details specified (the relevant commits)
jobs:
release:
runs-on: ${{ matrix.os }}
if: ${{ github.repository == 'alibaba/GraphAr' }}
strategy:
matrix:
os: [ubuntu-latest]

steps:
- name: Release
uses: "marvinpinto/action-automatic-releases@latest"
with:
repo_token: "${{ secrets.GITHUB_TOKEN }}"
prerelease: false
171 changes: 171 additions & 0 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ starting the implementation. There are often a number of ways to fix a problem
and it is important to find the right approach before spending time on a PR
that cannot be merged.

Minor Fixes
^^^^^^^^^^^^

Any functionality change should have a GitHub issue opened. For minor changes that
affect documentation, you do not need to open up a GitHub issue. Instead you can
prefix the title of your PR with "[Minor] " if meets the following guidelines:

* Grammar, usage and spelling fixes that affect no more than 2 files
* Documentation updates affecting no more than 2 files and not more
than 500 words.

Fork & create a branch
^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down Expand Up @@ -200,18 +211,178 @@ A PR can only be merged into main by a maintainer if:
Any maintainer is allowed to merge a PR if all of these conditions are
met.

Shipping a release (maintainers only)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Maintainers need to do the following to push out a release:

* Switch to the main branch and make sure it's up to date.

.. code:: shell

git checkout main
git pull upstream main

* Tag the release with a version number and push it to GitHub. Note that the version number should follow `semantic versioning <https://semver.org/#summary>`_. e.g.: v0.1.0.

.. code:: shell

git tag v0.1.0
git push upstream v0.1.0

* The release will be automatically built and published to GitHub by GitHub Actions. You can edit the release notes on `GitHub <https://github.com/alibaba/GraphAr/releases>`_ to add more details.

.. the reviewing part document is referred and derived from
.. https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#the-process-of-making-changes
Reviewing pull requests
-----------------------

All contributors who choose to review and provide feedback on Pull Requests have
a responsibility to both the project and the individual making the contribution.
Reviews and feedback must be helpful, insightful, and geared towards improving
the contribution as opposed to simply blocking it. Do not expect to be able to
block a pull request from advancing simply because you say "No" without giving
an explanation. Be open to having your mind changed. Be open to working with the
contributor to make the pull request better.

Reviews that are dismissive or disrespectful of the contributor or any other
reviewers are strictly counter to the `Code of Conduct`_ and will not be tolerated.

When reviewing a pull request, the primary goals are for the codebase to improve
and for the person submitting the request to succeed. Even if a pull request does
not land, the submitters should come away from the experience feeling like their
effort was not wasted or unappreciated. Every pull request from a new contributor
is an opportunity to grow the community.

Review a bit at a time
^^^^^^^^^^^^^^^^^^^^^^^

Do not overwhelm new contributors.

It is tempting to micro-optimize and make everything about relative performance,
perfect grammar, or exact style matches. Do not succumb to that temptation.

Focus first on the most significant aspects of the change:

1. Does this change make sense for GraphAr?
2. Does this change make GraphAr better, even if only incrementally?
3. Are there clear bugs or larger scale issues that need attending to?
4. Is the commit message readable and correct? If it contains a breaking change
is it clear enough?

When changes are necessary, *request* them, do not *demand* them, and do not
assume that the submitter already knows how to add a test or run a benchmark.

Specific performance optimization techniques, coding styles, and conventions
change over time. The first impression you give to a new contributor never does.

Nits (requests for small changes that are not essential) are fine, but try to
avoid stalling the pull request. Most nits can typically be fixed by the
GraphAr collaborator landing the pull request but they can also be an
opportunity for the contributor to learn a bit more about the project.

It is always good to clearly indicate nits when you comment: e.g.
:code:`Nit: change foo() to bar(). But this is not blocking.`

If your comments were addressed but were not folded automatically after new
commits or if they proved to be mistaken, please, `hide them <https://docs.github.com/en/communities/moderating-comments-and-conversations/managing-disruptive-comments#hiding-a-comment>`_
with the appropriate reason to keep the conversation flow concise and relevant.

Be aware of the person behind the code
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Be aware that *how* you communicate requests and reviews in your feedback can
have a significant impact on the success of the pull request. Yes, we may land
a particular change that makes GraphAr better, but the individual might just
not want to have anything to do with GraphAr ever again. The goal is not just
having good code.

Respect the minimum wait time for comments
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

There is a minimum waiting time which we try to respect for non-trivial
changes, so that people who may have important input in such a distributed
project are able to respond.

For non-trivial changes, pull requests must be left open for at least 48 hours.
Sometimes changes take far longer to review, or need more specialized review
from subject-matter experts. When in doubt, do not rush.

Trivial changes, typically limited to small formatting changes or fixes to
documentation, may be landed within the minimum 48 hour window.

Abandoned or stalled pull requests
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If a pull request appears to be abandoned or stalled, it is polite to first
check with the contributor to see if they intend to continue the work before
checking if they would mind if you took it over (especially if it just has
nits left). When doing so, it is courteous to give the original contributor
credit for the work they started (either by preserving their name and email
address) in the commit log, or by using an :code:`Author:` meta-data tag in the
commit.

Approving a change
^^^^^^^^^^^^^^^^^^^

Any GraphAr core collaborator (any GitHub user with commit rights in the
:code:`alibaba/GraphAr` repository) is authorized to approve any other contributor's
work. Collaborators are not permitted to approve their own pull requests.

Collaborators indicate that they have reviewed and approve of the changes in
a pull request either by using GitHub's Approval Workflow, which is preferred,
or by leaving an :code:`LGTM` ("Looks Good To Me") comment.

When explicitly using the "Changes requested" component of the GitHub Approval
Workflow, show empathy. That is, do not be rude or abrupt with your feedback
and offer concrete suggestions for improvement, if possible. If you're not
sure **how** a particular change can be improved, say so.

Most importantly, after leaving such requests, it is courteous to make yourself
available later to check whether your comments have been addressed.

If you see that requested changes have been made, you can clear another
collaborator's :code:`Changes requested` review.

Change requests that are vague, dismissive, or unconstructive may also be
dismissed if requests for greater clarification go unanswered within a
reasonable period of time.

Use :code:`Changes requested` to block a pull request from landing. When doing so,
explain why you believe the pull request should not land along with an
explanation of what may be an acceptable alternative course, if any.

Performance is not everything
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

GraphAr has always optimized for speed of execution. If a particular change
can be shown to make some part of GraphAr faster, it's quite likely to be
accepted.

That said, performance is not the only factor to consider. GraphAr also
optimizes in favor of not breaking existing code in the ecosystem, and not
changing working functional code just for the sake of changing.

If a particular pull request introduces a performance or functional
regression, rather than simply rejecting the pull request, take the time to
work *with* the contributor on improving the change. Offer feedback and
advice on what would make the pull request acceptable, and do not assume that
the contributor should already know how to do that. Be explicit in your
feedback.

Continuous integration testing
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

All pull requests that contain changes to code must be run through
continuous integration (CI) testing at `Github Actions <https://github.com/alibaba/GraphAr/actions>`_.

The pull request change will triggers a CI testing run. Ideally, the code change
acezen marked this conversation as resolved.
Show resolved Hide resolved
will pass ("be green") on all platform configurations supported by GraphAr.
This means that all tests pass and there are no linting errors. In reality,
however, it is not uncommon for the CI infrastructure itself to fail on specific
platforms ("be red"). It is vital to visually inspect the results of all failed ("red") tests
to determine whether the failure was caused by the changes in the pull request.

.. _Code of Conduct: https://github.com/alibaba/GraphAr/blob/main/CODE_OF_CONDUCT.md

.. _file an bug issue: https://github.com/alibaba/GraphAr/issues/new?assignees=&labels=Bug&template=bug_report.yml&title=%5BBug%5D%3A+%3Ctitle%3E
acezen marked this conversation as resolved.
Show resolved Hide resolved
Expand Down