From de76c3aadd2c17c7669892e66ff4159c6e20e8db Mon Sep 17 00:00:00 2001 From: Anders Ingemann Date: Sat, 10 May 2014 16:22:32 +0200 Subject: [PATCH] Move development guidelines into docs --- docs/common/fs.rst | 2 +- docs/guidelines.rst | 185 ++++++++++++++++++++++++++++++++++++++++++++ docs/index.rst | 1 + 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 docs/guidelines.rst diff --git a/docs/common/fs.rst b/docs/common/fs.rst index fdbfc51..70af71b 100644 --- a/docs/common/fs.rst +++ b/docs/common/fs.rst @@ -1,3 +1,3 @@ -Common volume representations +Volume representations ============================= diff --git a/docs/guidelines.rst b/docs/guidelines.rst new file mode 100644 index 0000000..cd798c9 --- /dev/null +++ b/docs/guidelines.rst @@ -0,0 +1,185 @@ +Development guidelines +====================== +The following guidelines should serve as general advice when +developing providers or plugins for bootstrap-vz. Keep in mind that +these guidelines are not rules , they are advice on how to better add +value to the bootstrap-vz codebase. + + ++ **The manifest should always fully describe the resulting image. The + outcome of a bootstrapping process should never depend on settings + specified elsewhere.** + + This allows others to easily reproduce any + setup other people are running and makes it possible to share + manifests. `The official debian EC2 images `_ + for example can be reproduced using the manifests available + in the manifest directory of bootstrap-vz. + ++ **The bootstrapper should always be able to run fully unattended.** + + For end users, this guideline minimizes the risk of errors. Any + required input would also be in direct conflict with the previous + guideline that the manifest should always fully describe the resulting + image. + + Additionally developers may have to run the bootstrap + process multiple times though, any prompts in the middle of that + process may significantly slow down the development speed. + ++ **The bootstrapper should only need as much setup as the manifest + requires.** + + Having to shuffle specific paths on the host into place + (e.g. ``/target`` has to be created manually) to get the bootstrapper + running is going to increase the rate of errors made by users. + Aim for minimal setup. + + Exceptions are of course things such as the path to + the VirtualBox Guest Additions ISO or tools like ``parted`` that + need to be installed on the host. + ++ **Roll complexity into which tasks are added to the tasklist.** + + If a ``run()`` function checks whether it should do any work or simply be + skipped, consider doing that check in ``resolve_tasks()`` instead and + avoid adding that task alltogether. This allows people looking at the + tasklist in the logfile to determine what work has been performed. If + a task says it will modify a file but then bails , a developer may get + confused when looking at that file after bootstrapping. He could + conclude that the file has either been overwritten or that the + search & replace does not work correctly. + ++ **Control flow should be directed from the task graph.** + + Avoid creating complicated ``run()`` functions. If necessary, split up + a function into two semantically separate tasks. + + This allows other tasks to interleave with the control-flow and add extended + functionality (e.g. because volume creation and mounting are two + separate tasks, `the prebootstrapped plugin + `_ + can replace the volume creation task with a task of its own that + creates a volume from a snapshot instead, but still reuse the mount task). + ++ **Task classes should be treated as decorated run() functions, they + should not have any state** + + Thats what the BootstrapInformation object is for. + ++ **Only add stuff to the BootstrapInformation object when really necessary.** + + This is mainly to avoid clutter. + ++ **Use a json-schema to check for allowed settings** + The json-schema may be verbose but it keeps the bulk of check work outside the + python code, which is a big plus when it comes to readability. This of + course only applies bas long as the checks are simple. You can of + course fall back to doing the check in python when that solution is + considerably less complex. + ++ **When invoking external programs, use long options whenever possible** + + This makes the commands a lot easier to understand, since + the option names usually hint at what they do. + ++ **When invoking external programs, don't use full paths, rely on ``$PATH``** + + This increases robustness when executable locations change. + Example: Use ``log_call(['wget', ...])`` instead of ``log_call(['/usr/bin/wget', ...])``. + + +Commandline switches +-------------------- +As a developer, there are commandline switches available which can +make your life a lot easier. + ++ ``--debug``: Enables debug output in the console. This includes output + from all commands that are invoked during bootstrapping. ++ ``--pause-on-error``: Pauses the execution when an exception occurs + before rolling back. This allows you to debug by inspecting the volume + at the time the error occured. ++ ``--dry-run``: Prevents the ``run()`` function from being called on all + tasks. This is useful if you want to see whether the task order is + correct. + + +Logfile +------- +Every run creates a new logfile in the ``logs/`` directory. The filename +for each run consists of a timestamp (``%Y%m%d%H%M%S``) and the basename +of the manifest used. The log also contains debugging statements +regardless of whether the ``--debug`` switch was used. + + +Inner workings +-------------- + +Tasks +~~~~~ +At its core bootstrap-vz is based on tasks that perform units of work. +By keeping those tasks small and with a solid structure built around +them a high degree of flexibility can be achieved. To ensure that +tasks are executed in the right order, each task is placed in a +dependency graph where directed edges dictate precedence. Each task is +a simple class that defines its predecessor tasks and successor tasks +via attributes. Here is an example: + +:: + + class MapPartitions(Task): + description = 'Mapping volume partitions' + phase = phases.volume_preparation + predecessors = [PartitionVolume] + successors = [filesystem.Format] + + @classmethod + def run(cls, info): + info.volume.partition_map.map(info.volume) + +In this case the attributes define that the task at hand should run +after the ``PartitionVolume`` task — i.e. after volume has been +partitioned (``predecessors``) — but before formatting each +partition (``successors``). +It is also placed in the ``volume_preparation`` phase. +Phases are ordered and group tasks together. All tasks in a phase are +run before proceeding with the tasks in the next phase. They are a way +of avoiding the need to list 50 different tasks as predecessors and +successors. + +The final task list that will be executed is computed by enumerating +all tasks in the package, placing them in the graph and +`sorting them topoligcally `_. +Subsequently the list returned is filtered to contain only the tasks the +provider and the plugins added to the taskset. + + +System abstractions +~~~~~~~~~~~~~~~~~~~ +There are several abstractions in bootstrap-vz that make it possible +to generalize things like volume creation, partitioning, mounting and +package installation. As a rule these abstractions are located in the +``base/`` folder, where the manifest parsing and task ordering algorithm +are placed as well. + + +Coding style +------------ +bootstrap-vz is coded to comply closely with the PEP8 style +guidelines. There however a few exceptions: + ++ Max line length is 110 chars, not 80. ++ Multiple assignments may be aligned with spaces so that the = match + vertically. ++ Ignore ``E101``: Indent with tabs and align with spaces ++ Ignore ``E221 & E241``: Alignment of assignments ++ Ignore ``E501``: The max line length is not 80 characters ++ Ignore ``W191``: Indent with tabs not spaces + +The codebase can be checked for any violations quite easily with: +:: + find . -name '*.py' | /usr/bin/grep -v minify_json | xargs pep8 --ignore=E101,E221,E241,E501,W191 + +Note: ``common/minify_json.py`` is ignored since it is foreign code. + diff --git a/docs/index.rst b/docs/index.rst index c9db652..9e928b0 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -15,6 +15,7 @@ Contents: common/index plugins/index providers/index + guidelines Indices and tables ==================