From 2576379df556e5eecd8fd2dd9f5138fc3c8aeefb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Fri, 2 Nov 2018 15:10:18 +0200 Subject: [PATCH] Dev. docs.: Add git workflows for branching, PR and reviewing --- docs/contributors/environment.rst | 199 ++++++++++++++++++++++++++++-- docs/contributors/guide.rst | 2 + 2 files changed, 194 insertions(+), 7 deletions(-) diff --git a/docs/contributors/environment.rst b/docs/contributors/environment.rst index a79f6bb9..b539293b 100644 --- a/docs/contributors/environment.rst +++ b/docs/contributors/environment.rst @@ -1,20 +1,117 @@ Development environment ======================= +Git +--- + +Before any partaking in development, you will need to fork the Mailu repository on GitHub. +For this you will need a `GitHub`_ account. GitHub has excellent documentation on: + +#. How to `fork a repo`_ and set upstream (Mailu); +#. Keeping your fork `synced`_; +#. Sending a `pull request`_. + +Working on Mailu usually requires you to clone (download) your fork to your work station and +create a branch. From here you can work on Mailu. When done, create a commit and push the +branch to your GitHub repository. Then, on GitHub you can create a "pull request". +Please make sure you have read the :ref:`git_workflow` section of the *Development guidelines* +before submitting any pull requests. + +.. note:: It is strongly advised to **never** modify the ``master`` branch of your fork. + This will make it impossible to sync your fork with upstream and creating new (and clean) + branches! This includes never merging other branches from yourself or other users into your + ``master``. If you want to do that, create a separate branch for it. + +Short work flow example +``````````````````````` + +.. code-block:: bash + + git clone https://github.com//Mailu.git + cd Mailu + git add remote upstream https://github.com/Mailu/Mailu.git + git checkout -b fix-something master + +Work on the code as desired. Before doing a commit, you should at least build +and run the containers. Keep reading this guide for more information. After this, +continue to commit and send a PR. + +.. code-block:: bash + + git commit -a + #Enter commit message in editor, save and close. + git push --set-upstream origin fix-something + +Now you can go to your GitHub page, select the new branch and "send pull request". + +Updating your fork +`````````````````` + +The Mailu ``master`` branch is an ever evolving target. It is important that newly +created branches originate from the latest ``upstream/master``. In order to do so, you will +need to `sync your fork`__: + +.. code-block:: bash + + git fetch --all + git checkout master + git merge upstream/master + +If you kept your master branch clean, this should fast-forward it to the latest upstream version. +Likewise, if you worked on your branch for a longer amount of time, it is advised to merge the +latest ``upstream/master`` into the branch. + +.. code-block:: bash + + git checkout my-old-branch + git merge upstream/master + +Now, git won't fast forward but write a merge commit. Typically you can accept the commit message +presented. Read the output if there are any merge conflicts. In ``git status`` you can find the files +that need editing to have the desired contents. Also, it will tell you how to mark them as resolved. + +Optionally, you can ``git push`` after any of above merges to propagate them to GitHub. + +__ `synced`_ + +Bad habits +``````````` + +Some bad habits from users that we are sometimes confronted with. Please refrain yourself from: + +- ``git reset REF`` and ``git push --force`` after submitting a PR. +- Merge a branch (other then master) into yours and submitting a PR before that other branch got + merged into master. It will cause you to submit commits someone else wrote and are probably outside + the subject of your PR. (There are valid cases however, but take care!) +- ``git reset REF`` after merging ``upstream/master`` into your branch. It will unstage **all** + changed files that where updated in the merge. Your will have to clean up all of them + (don't delete!) using ``git checkout -- ``. And take care not to do that to the files you + have modified. However, it can be that the merge modified some other lines then yours. You'll have + to make sure there will be no conflicts when you are submitting this messed up branch to Mailu! You + get the point, I hope. +- ``git rebase`` on a branch that is pull-requested. Others will not be able to see you modified the + branch and it messes with the order of commits, compared to a merge. It might break things after we + have conducted tests. + +.. _`GitHub`: https://github.com/ +.. _`fork a repo`: https://help.github.com/articles/fork-a-repo/ +.. _`synced`: https://help.github.com/articles/syncing-a-fork/ +.. _`pull request`: https://help.github.com/articles/about-pull-requests/ + Docker containers ----------------- -The development environment is quite similar to the production one. You should always use -the ``master`` version when developing. +The development environment is quite similar to the production one. Building images ``````````````` -We supply a separate ``test/build.yml`` file for -convenience. To build all Mailu containers: +We supply a separate ``test/build.yml`` file for convenience. +After cloning the git repository to your workstation, you can build the images: .. code-block:: bash + cd Mailu docker-compose -f tests/build.yml build The ``build.yml`` file has two variables: @@ -73,10 +170,96 @@ Finally, if you need to install packages inside the containers for debugging: docker-compose exec admin apk add --no-cache package-name +Reviewing +--------- + +System requirements +``````````````````` + +Reviewing pull requests requires some additional git setup. First, for 90% of the review jobs, +you will need a PC or server that can expose all Mailu ports to the outside world. Also, a valid +domain name would be required. This can be a simple free DynDNS account. Do not use a production +server, as there are cases where data corruption occurs and you need to delete the ``/mailu`` +directory structure. + +If you do no posses the resources, but want to become an involved tester/reviewer. +Please contact `muhlemmer on Matrix`_. +He can provide access to a testing server, if a thrust relation can be established. + +.. _`muhlemmer on Matrix`: https://matrix.to/#/@muhlemmer:matrix.org + +Preparations +```````````` + +#. Setup `Git`_ the same way as on a development PC. It is advised to keep ``origin`` as your + own repository and ``upstream`` as the one from Mailu. This will avoid confusion; +#. You will need a ``docker-compose.yml`` and ``.env``, set up for the test server; +#. Make sure that the build ``$VERSION`` corresponds with those files. + +Add the sender +`````````````` + +Replace ```` with the repository name the PR is sent from. + +.. code-block:: bash + + git remote add https://github.com//Mailu.git + +Merge conflicts +``````````````` + +Before proceeding, check the PR page in the bottom. It should not indicate a merge conflict. +If there are merge conflicts, you have 2 options: + +#. Do a review "request changes" and ask the author to resolve the merge conflict. +#. Solve the merge conflict yourself on Github, using the web editor. + +If it can't be done in the web editor, go for option 1. Unless you want to go through the trouble of +importing the branch into your fork, do the merge and send a PR to the repository of the *sender*. + +Merge the PR locally +``````````````````````` + +When someone sends a PR, you need merge his PR into master locally. This example will put you in a +"detached head" state and do the merge in that state. Any commits done in this state will be lost +forever when you checkout a "normal" branch. This is exactly what we want, as we do not want to mess +with our repositories. This is just a test run. + +The following must be done on every PR or after every new commit to an existing PR: +1. Fetch the latest status of all the remotes. +2. List all local and remote available branches (this is not needed, but very helpful at times) +3. Checkout ``upstream/master`` +4. Merge ``upstream/master`` with ``SENDER/branch`` + +.. code-block:: bash + + git fetch --all + git checkout upstream/master + # ...You are in 'detached HEAD' state.... (bla bla bla) + git branch -a + # Hit `q` to exit the viewer, if it was opened. Uses arrows up/down for scrolling. + git merge kaiyou/fix-sender-checks + +If git opens a editor for a commit message just save and exit as-is. If you have a merge conflict, +see above and do the complete procedure from ``git fetch`` onward again. + +Test +```` + +You can now build and run the containers for testing. See the "`Docker containers`_" section for +instructions. Play around. See if (external) mails work. Check for whatever functionality the PR is +trying to fix. When happy, you can approve the PR. When running into failures, mark the review as +"request changes" and try to provide as much as possible details on the failure. +(Logs, error codes form clients etc). + +.. note:: Github marks positive reviews as obsolete when a new commit is added to a PR. + This requires a new review from your side. + Web administration ------------------ -The administration Web interface requires a proper dev environment that can easily be setup using ``virtualenv`` (make sure you are using Python 3) : +The administration Web interface requires a proper dev environment that can easily be setup using +``virtualenv`` (make sure you are using Python 3) : .. code-block:: bash @@ -105,7 +288,8 @@ of the screen, that you can open to access query details, internal variables, et Documentation ------------- -Documentation is maintained in the ``docs`` directory and are maintained as `reStructuredText`_ files. It is possible to run a local documentation server for reviewing purposes, using Docker: +Documentation is maintained in the ``docs`` directory and are maintained as `reStructuredText`_ +files. It is possible to run a local documentation server for reviewing purposes, using Docker: .. code-block:: bash @@ -116,6 +300,7 @@ Documentation is maintained in the ``docs`` directory and are maintained as `reS In a local build Docker always assumes the version to be master. You can read the local documentation by navigating to http://localhost:8080/master. -.. note:: After modifying the documentation, the image needs to be rebuild and the container restarted for the changes to become visible. +.. note:: After modifying the documentation, the image needs to be rebuild and the container + restarted for the changes to become visible. .. _`reStructuredText`: http://docutils.sourceforge.net/rst.html diff --git a/docs/contributors/guide.rst b/docs/contributors/guide.rst index 705af469..865fca94 100644 --- a/docs/contributors/guide.rst +++ b/docs/contributors/guide.rst @@ -13,6 +13,8 @@ Docker best practices and be as generic as possible : - interesting settings should be available as environment variables - base images should be well-trusted (officiel Alpine or Debian for instance). +.. _git_workflow: + Git workflow ------------