diff options
Diffstat (limited to 'doc/dev/developer_guide/basic-workflow.rst')
| -rw-r--r-- | doc/dev/developer_guide/basic-workflow.rst | 225 |
1 files changed, 129 insertions, 96 deletions
diff --git a/doc/dev/developer_guide/basic-workflow.rst b/doc/dev/developer_guide/basic-workflow.rst index 4a6913fb89a..1dfb0029d3d 100644 --- a/doc/dev/developer_guide/basic-workflow.rst +++ b/doc/dev/developer_guide/basic-workflow.rst @@ -1,7 +1,7 @@ Basic Workflow ============== -The following chart illustrates basic development workflow: +The following chart illustrates the basic Ceph development workflow: .. ditaa:: @@ -28,60 +28,62 @@ The following chart illustrates basic development workflow: | pull request | git push \-------------/ \--------------/ -Below we present an explanation of this chart. The explanation is written -with the assumption that you, the reader, are a beginning developer who -has an idea for a bugfix, but do not know exactly how to proceed. Watch -the `Getting Started with Ceph Development -<https://www.youtube.com/watch?v=t5UIehZ1oLs>`_ video for -a practical summary of the same. +The below explanation is written with the assumption that you, the reader, are +a new contributor who has an idea for a bugfix or enhancement, but do not know +exactly how to proceed. Watch the `Getting Started with Ceph Development +<https://www.youtube.com/watch?v=t5UIehZ1oLs>`_ video for a practical summary +of this workflow. Update the tracker ------------------ -Before you start, you should know the :ref:`issue-tracker` number of the bug -you intend to fix. If there is no tracker issue, now is the time to create -one. +Before you start, you should know the :ref:`issue-tracker` (Redmine) number +of the bug you intend to fix. If there is no tracker issue, now is the time to +create one for code changes. Straightforward documentation cleanup does +not necessarily require a corresponding tracker issue. However, an issue +(ticket) should be created if one is adding new documentation chapters or +files, or for other substantial changes. -The tracker is there to explain the issue (bug) to your fellow Ceph -developers and keep them informed as you make progress toward resolution. -To this end, then, provide a descriptive title as well as sufficient -information and details in the description. +The tracker ticket serves to explain the issue (bug) to your fellow Ceph +developers and keep them informed as you make progress toward resolution. To +this end, please provide a descriptive title and write appropriate information +and details into the description. When composing the ticket's title, consider "If I +want to search for this ticket two years from now, what keywords will I search +for?" If you have sufficient tracker permissions, assign the bug to yourself by -changing the ``Assignee`` field. If your tracker permissions have not yet -been elevated, simply add a comment to the issue with a short message like -"I am working on this issue". +setting the ``Assignee`` field. If your tracker permissions have not been +elevated, simply add a comment with a short message like "I am working on this +issue". Upstream code ------------- -This section, and the ones that follow, correspond to the nodes in the -above chart. +This section, and the ones that follow, correspond to nodes in the above chart. -The upstream code lives in https://github.com/ceph/ceph.git, which is -sometimes referred to as the "upstream repo", or simply "upstream". As the -chart illustrates, we will make a local copy of this code, modify it, test -our modifications, and submit the modifications back to the upstream repo -for review. +The upstream code is found at https://github.com/ceph/ceph.git, which is known +as the "upstream repo", or simply "upstream". As the chart shows, we will make +a local copy of this repository, modify it, test our modifications, then submit +the modifications for review and merging. A local copy of the upstream code is made by -1. forking the upstream repo on GitHub, and -2. cloning your fork to make a local working copy +1. Forking the upstream repo on GitHub, and +2. Cloning your fork to make a local working copy -See the `the GitHub documentation +See the `GitHub documentation <https://help.github.com/articles/fork-a-repo/#platform-linux>`_ for detailed instructions on forking. In short, if your GitHub username is -"mygithubaccount", your fork of the upstream repo will show up at +"mygithubaccount", your fork of the upstream repo will appear at https://github.com/mygithubaccount/ceph. Once you have created your fork, -you clone it by doing: +clone it by running: .. prompt:: bash $ git clone https://github.com/mygithubaccount/ceph -While it is possible to clone the upstream repo directly, in this case you -must fork it first. Forking is what enables us to open a `GitHub pull +While it is possible to clone the upstream repo directly, for the Ceph workflow +you must fork it first. Forking is what enables us to open a `GitHub pull request`_. For more information on using GitHub, refer to `GitHub Help @@ -90,13 +92,25 @@ For more information on using GitHub, refer to `GitHub Help Local environment ----------------- -In the local environment created in the previous step, you now have a -copy of the ``master`` branch in ``remotes/origin/master``. Since the fork +In the local environment created in the previous step, you now have a copy of +the ``master`` branch in ``remotes/origin/master``. This fork (https://github.com/mygithubaccount/ceph.git) is frozen in time and the upstream repo (https://github.com/ceph/ceph.git, typically abbreviated to -``ceph/ceph.git``) is updated frequently by other developers, you will need -to sync your fork periodically. To do this, first add the upstream repo as -a "remote" and fetch it +``ceph/ceph.git``) is updated frequently by other contributors, you must sync +your fork periodically. Failure to do so may result in your commits and pull +requests failing to merge because they refer to file contents that have since +changed. + +First, ensure that you have properly configured your local git environment with +your name and email address. Skip this step if you have already configured this +information. + +.. prompt:: bash $ + + git config user.name "FIRST_NAME LAST_NAME" + git config user.email "MY_NAME@example.com" + +Now add the upstream repo as a "remote" and fetch it: .. prompt:: bash $ @@ -107,10 +121,10 @@ Fetching downloads all objects (commits, branches) that were added since the last sync. After running these commands, all the branches from ``ceph/ceph.git`` are downloaded to the local git repo as ``remotes/ceph/$BRANCH_NAME`` and can be referenced as -``ceph/$BRANCH_NAME`` in certain git commands. +``ceph/$BRANCH_NAME`` in local git commands. For example, your local ``master`` branch can be reset to the upstream Ceph -``master`` branch by doing +``master`` branch by running .. prompt:: bash $ @@ -118,7 +132,7 @@ For example, your local ``master`` branch can be reset to the upstream Ceph git checkout master git reset --hard ceph/master -Finally, the ``master`` branch of your fork can then be synced to upstream +Finally, the ``master`` branch of your fork is synced to the upstream master by .. prompt:: bash $ @@ -128,7 +142,7 @@ master by Bugfix branch ------------- -Next, create a branch for the bugfix: +Next, create a branch for your bugfix: .. prompt:: bash $ @@ -136,29 +150,31 @@ Next, create a branch for the bugfix: git checkout -b fix_1 git push -u origin fix_1 -This creates a ``fix_1`` branch locally and in our GitHub fork. At this -point, the ``fix_1`` branch is identical to the ``master`` branch, but not -for long! You are now ready to modify the code. +This creates a ``fix_1`` branch locally and in our GitHub fork. At this point, +the ``fix_1`` branch is identical to the ``master`` branch, but not for long! +You are now ready to modify the code. Be careful to always run `git checkout +master` first, otherwise you may find commits from an unrelated branch mixed +with your new work. Fix bug locally --------------- -At this point, change the status of the tracker issue to "In progress" to -communicate to the other Ceph developers that you have begun working on a -fix. If you don't have permission to change that field, your comment that -you are working on the issue is sufficient. +Now change the status of the tracker issue to "In progress" to communicate to +other Ceph contributors that you have begun working on a fix. This helps avoid +duplication of effort. If you don't have permission to change that field, your +previous comment that you are working on the issue is sufficient. -Possibly, your fix is very simple and requires only minimal testing. -More likely, it will be an iterative process involving trial and error, not -to mention skill. An explanation of how to fix bugs is beyond the -scope of this document. Instead, we focus on the mechanics of the process -in the context of the Ceph project. +Your fix may be very simple and require only minimal testing. More likely, +this will be an iterative process involving trial and error, not to mention +skill. An explanation of how to fix bugs is beyond the scope of this +document. Instead, we focus on the mechanics of the process in the context of +the Ceph project. -A detailed discussion of the tools available for validating your bugfixes, +For a detailed discussion of the tools available for validating bugfixes, see the chapters on testing. -For now, let us just assume that you have finished work on the bugfix and -that you have tested it and believe it works. Commit the changes to your local +For now, let us just assume that you have finished work on the bugfix, that +you have tested, and that you believe it works. Commit the changes to your local branch using the ``--signoff`` option .. prompt:: bash $ @@ -174,44 +190,48 @@ and push the changes to your fork GitHub pull request ------------------- -The next step is to open a GitHub pull request. The purpose of this step is -to make your bugfix available to the community of Ceph developers. They -will review it and may do additional testing on it. +The next step is to open a GitHub pull request (PR). This makes your bugfix +visible to the community of Ceph contributors. They will review it and may +perform additional testing and / or request changes. -In short, this is the point where you "go public" with your modifications. -Psychologically, you should be prepared to receive suggestions and -constructive criticism. Don't worry! In our experience, the Ceph project is -a friendly place! +This is the point where you "go public" with your modifications. Be prepared +to receive suggestions and constructive criticism in the form of comments +within the PR. Don't worry! The Ceph project is a friendly place! -If you are uncertain how to use pull requests, you may read +If you are uncertain how to create and manage pull requests, you may read `this GitHub pull request tutorial`_. .. _`this GitHub pull request tutorial`: https://help.github.com/articles/using-pull-requests/ -For some ideas on what constitutes a "good" pull request, see +For ideas on what constitutes a "good" pull request, see the `Git Commit Good Practice`_ article at the `OpenStack Project Wiki`_. .. _`Git Commit Good Practice`: https://wiki.openstack.org/wiki/GitCommitMessages .. _`OpenStack Project Wiki`: https://wiki.openstack.org/wiki/Main_Page +and our own `Submitting Patches <https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst>`_ document. + Once your pull request (PR) is opened, update the :ref:`issue-tracker` by -adding a comment to the bug pointing the other developers to your PR. The -update can be as simple as:: +adding a comment directing other contributors to your PR. The comment can be +as simple as:: *PR*: https://github.com/ceph/ceph/pull/$NUMBER_OF_YOUR_PULL_REQUEST Automated PR validation ----------------------- -When your PR hits GitHub, the Ceph project's `Continuous Integration (CI) -<https://en.wikipedia.org/wiki/Continuous_integration>`_ -infrastructure will test it automatically. At the time of this writing -(September 2020), the automated CI testing included five tests to check that the -commits in the PR are properly signed (see :ref:`submitting-patches`), to check that the documentation builds, to check that the submodules are unmodified, to check that the API is in order, and a :ref:`make-check` test. +When your PR is created or updated, the Ceph project's `Continuous Integration +(CI) <https://en.wikipedia.org/wiki/Continuous_integration>`_ infrastructure +will test it automatically. At the time of this writing (September 2020), the +automated CI testing included five tests to check that the commits in the PR +are properly signed (see :ref:`submitting-patches`), to check that the +documentation builds, to check that the submodules are unmodified, to check +that the API is in order, and a :ref:`make-check` test. Additional tests may +be performed depending on which files are modified by your PR. The :ref:`make-check`, builds the PR and runs it through a battery of -tests. These tests run on machines operated by the Ceph Continuous +tests. These tests run on servers operated by the Ceph Continuous Integration (CI) team. When the tests complete, the result will be shown on GitHub in the pull request itself. @@ -224,25 +244,32 @@ Notes on PR make check test The GitHub :ref:`make check<make-check>` test is driven by a Jenkins instance. Jenkins merges your PR branch into the latest version of the base branch before -starting the build. This means that you don't have to rebase the PR to pick up any fixes. +starting tests. This means that you don't have to rebase the PR to pick up any fixes. -You can trigger the PR tests at any time by adding a comment to the PR - the +You can trigger PR tests at any time by adding a comment to the PR - the comment should contain the string "test this please". Since a human subscribed -to the PR might interpret that as a request for him or her to test the PR, -we recommend that you address Jenkins directly in the request; for example, write "jenkins retest this please". - -If there is a build failure and you aren't sure what caused it, check the :ref:`make check<make-check>` log. To access it, first click on "details" (next -to the :ref:`make check<make-check>` test in the PR) to enter the Jenkins -web GUI. Then click on "Console Output" (on the left). - -Jenkins is set up to grep the log for strings known to have been associated -with :ref:`make check<make-check>` failures in the past. However, there is no guarantee that the known strings are associated with any given :ref:`make check<make-check>` failure. You'll have to dig into the log to determine the cause of the failure. +to the PR might interpret that as a request for him or her to test the PR, we +recommend that you address Jenkins directly. For example, write "jenkins retest +this please". For efficiency a single re-test can also be requested with +e.g. "jenkins test signed". For reference, a list of these requests is +automatically added to the end of each new PR's description. + +If there is a build failure and you aren't sure what caused it, check the +:ref:`make check<make-check>` log. To access it, click on the "details" (next +to the :ref:`make check<make-check>` test in the PR) link to enter the Jenkins web +GUI. Then click on "Console Output" (on the left). + +Jenkins is configured to search logs for strings known to have been associated +with :ref:`make check<make-check>` failures in the past. However, there is no +guarantee that these known strings are associated with any given +:ref:`make check<make-check>` failure. You'll have to read through the log to determine the +cause of your specific failure. Integration tests AKA ceph-qa-suite ----------------------------------- -Since Ceph is a complex beast, it may also be necessary to test your fix to -see how it behaves on real clusters running either on real or virtual +Since Ceph is complex, it may be necessary to test your fix to +see how it behaves on real clusters running on physical or virtual hardware. Tests designed for this purpose live in the `ceph/qa sub-directory`_ and are run via the `teuthology framework`_. @@ -252,7 +279,7 @@ sub-directory`_ and are run via the `teuthology framework`_. The Ceph community has access to the `Sepia lab <https://wiki.sepia.ceph.com/doku.php>`_ where :ref:`testing-integration-tests` can be -run on real hardware. Other developers may add tags like "needs-qa" to your +run on physical hardware. Other developers may add tags like "needs-qa" to your PR. This allows PRs that need testing to be merged into a single branch and tested all at the same time. Since teuthology suites can take hours (even days in some cases) to run, this can save a lot of time. @@ -267,7 +294,7 @@ Code review Once your bugfix has been thoroughly tested, or even during this process, it will be subjected to code review by other developers. This typically -takes the form of correspondence in the PR itself, but can be supplemented +takes the form of comments in the PR itself, but can be supplemented by discussions on :ref:`irc` and the :ref:`mailing-list`. Amending your PR @@ -276,7 +303,7 @@ Amending your PR While your PR is going through testing and `Code Review`_, you can modify it at any time by editing files in your local branch. -After the changes are committed locally (to the ``fix_1`` branch in our +After updates are committed locally (to the ``fix_1`` branch in our example), they need to be pushed to GitHub so they appear in the PR. Modifying the PR is done by adding commits to the ``fix_1`` branch upon @@ -290,11 +317,16 @@ will need to force push your branch with: git push --force origin fix_1 +Why do we take these extra steps instead of simply adding additional commits +the the PR? It is best practice for a PR to consist of a single commit; this +makes for clean history, eases peer review of your changes, and facilitates +merges. In rare circumstances it also makes it easier to cleanly revert +changes. + Merge ----- -The bugfixing process culminates when one of the project leads decides to -merge your PR. +The bugfix process completes when a project lead merges your PR. When this happens, it is a signal for you (or the lead who merged the PR) to change the :ref:`issue-tracker` status to "Resolved". Some issues may be @@ -314,22 +346,22 @@ This is the most basic form of a merge commit:: This consists of two parts: -#. The title of the commit of the pull request to be merged. +#. The title of the commit / PR to be merged. #. The name and email address of the reviewer. Enclose the reviewer's email address in angle brackets. Using .githubmap to Find a Reviewer's Email Address ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -If you cannot find the email address of the reviewer on his or her github +If you cannot find the email address of the reviewer on his or her GitHub page, you can look it up in the **.githubmap** file, which can be found in the repository at **/ceph/.githubmap**. Using "git log" to find a Reviewer's Email Address ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -If you cannot find a reviewer's email address by using the above methods, -you can search the git log for their email address. Reviewers are likely -to have committed something before, and as long as they have committed -something, the git log will probably contain their email address. +If you cannot find a reviewer's email address by using the above methods, you +can search the git log for their email address. Reviewers are likely to have +committed something before. If they have made previous contributions, the git +log will probably contain their email address. Use the following command @@ -350,3 +382,4 @@ the **ptl-tool** have the following form:: client: move client_lock to _unmount() client: add timer_lock support Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> + |