contribution: restructure entire chapter

- move subchapters to sub-sub chapters and try to have more internal
  references to e.g. commit rules.

1 files changed, 456 insertions, 9 deletions

diff --git a/docs/contributing/development.rst b/docs/contributing/development.rst
index 3ac9cf6f..61c45dba 100644
--- a/docs/contributing/development.rst
+++ b/docs/contributing/development.rst
@@ -20,7 +20,6 @@ https://github.com/vyos/vyos-build
 
 The README.md file will guide you to use the this top level repository.
 
-
 Submit a patch
 --------------
 
@@ -31,9 +30,10 @@ eases the automatic generation of a changelog file.
 A good approach for writing commit messages is actually to have a look at the
 file(s) history by invoking ``git log path/to/file.txt``.
 
+.. _prepare_commit:
 
-Preparding patch/commit
-^^^^^^^^^^^^^^^^^^^^^^^
+Preparing patch/commit
+^^^^^^^^^^^^^^^^^^^^^^
 
 In a big system, such as VyOS, that is comprised of multiple components, it's
 impossible to keep track of all the changes and bugs/feature requests in one's
@@ -81,12 +81,12 @@ post: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
 
   * All text of the commit message should be wrapped at 72 characters if
     possible which makes reading commit logs easier with ``git log`` on a
-	standard terminal (which happens to be 80x25)
+    standard terminal (which happens to be 80x25)
 
   * If applicable a reference to a previous commit should be made linking
     those commits nicely when browsing the history: ``After commit abcd12ef
-	("snmp: this is a headline") a Python import statement is missing,
-	throwing the follwoing exception: ABCDEF``
+    ("snmp: this is a headline") a Python import statement is missing,
+    throwing the follwoing exception: ABCDEF``
 
 * Always use the ``-x`` option to the ``git cherry-pick`` command when back or
   forward porting an individual commit. This automatically appends the line:
@@ -107,7 +107,6 @@ Limits:
 Please submit your patches using the well-known GitHub pull-request against our
 repositories found in the VyOS GitHub organisation at https://github.com/vyos
 
-
 Determining package for a fix
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -125,7 +124,6 @@ This means the file in question (``/opt/vyatta/sbin/vyatta-update-webproxy.pl``)
 is located in the ``vyatta-webproxy`` package which can be found here:
 https://github.com/vyos/vyatta-webproxy
 
-
 Fork repository to submit a Patch
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -162,7 +160,6 @@ record them in your created Git commit:
 
 * Submit the patch ``git push`` and create the GitHub pull-request.
 
-
 Attach patch to Phabricator task
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -174,4 +171,454 @@ commits and send it to maintainers@vyos.net or attach it directly to the bug
 * Export last commit to patch file: ``git format-patch`` or export the last two
   commits into its appropriate patch files: ``git format-patch -2``
 
+Coding Guidelines
+-----------------
+
+Like any other project we have some small guidelines about our source code, too.
+The rules we have are not there to punish you - the rules are in place to help
+us all. By having a consistent coding style it becomes very easy for new
+and also longtime contributors to navigate through the sources and all the
+implied logic of any one source file..
+
+Python 3 **shall** be used. How long can we keep Python 2 alive anyway? No
+considerations for Python 2 compatibility **should** be taken at any time.
+
+Formatting
+^^^^^^^^^^
+
+* Python: Tabs **shall not** be used. Every indentation level should be 4 spaces
+* XML: Tabs **shall not** be used. Every indentation level should be 2 spaces
+
+.. note: There are extensions to e.g. VIM (xmllint) which will help you to get
+   your indention levels correct. Add to following to your .vimrc file:
+   ``au FileType xml setlocal equalprg=xmllint\ --format\ --recover\ -\
+   2>/dev/null`` now you can call the linter using ``gg=G`` in command mode.
+
+Text generation
+###############
+
+Template processor **should** be used for generating config files. Built-in
+string formatting **may** be used for simple line-oriented formats where every
+line is self-contained, such as iptables rules. Template processor **must** be
+used for structured, multi-line formats such as those used by ISC DHCPd.
+
+The default template processor for VyOS code is Jinja2_.
+
+Summary
+#######
+
+When modifying the source code, remember these rules of the legacy elimination
+campaign:
+
+* No new features in Perl
+* No old style command definitions
+* No code incompatible with Python3
+
+Python
+------
+
+The switch to the Python programming language for new code is not merely a
+change of the language, but a chance to rethink and improve the programming
+approach.
+
+Let's face it: VyOS is full of spaghetti code where logic for reading the VyOS
+config, generating daemon configs, and restarting processes is all mixed up.
+
+Python (or any other language, for that matter) does not provide automatic
+protection from bad design, so we need to also devise design guidelines and
+follow them to keep the system extensible and maintainable.
+
+But we are here to assist you and want to guide you through how you can become
+a good VyOS contributor. The rules we have are not there to punish you - the
+rules are in place to help us all. What does it mean? By having a consistent
+coding style it becomes very easy for new contributors and also longtime
+contributors to navigate through the sources and all the implied logic of
+the spaghetti code.
+
+Please use the following template as good starting point when developing new
+modules or even rewrite a whole bunch of code in the new style XML/Pyhon
+interface.
+
+Configuration script structure and behaviour
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Your configuration script or operation mode script which is also written in
+Python3 should have a line break on 80 characters. This seems to be a bit odd
+nowadays but as some people also work remotly or programm using vi(m) this is
+a fair good standard which I hope we can rely on.
+
+In addition this also helps when browsing the GitHub codebase on a mobile
+device if you happen to be a crazy scientist.
+
+.. code-block:: python
+
+  #!/usr/bin/env python3
+  #
+  # Copyright (C) 2019 VyOS maintainers and contributors
+  #
+  # This program is free software; you can redistribute it and/or modify
+  # it under the terms of the GNU General Public License version 2 or later as
+  # published by the Free Software Foundation.
+  #
+  # This program is distributed in the hope that it will be useful,
+  # but WITHOUT ANY WARRANTY; without even the implied warranty of
+  # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+  # GNU General Public License for more details.
+  #
+  # You should have received a copy of the GNU General Public License
+  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+  import sys
+
+  from vyos.config import Config
+  from vyos.util import ConfigError
+
+  def get_config():
+      vc = Config()
+      # Convert the VyOS config to an abstract internal representation
+      config = ...
+      return config
+
+  def verify(config):
+      # Verify that configuration is valid
+      if invalid:
+          raise ConfigError("Descriptive message")
+      return True
+
+  def generate(config):
+      # Generate daemon configs
+      pass
+
+  def apply(config):
+      # Apply the generated configs to the live system
+      pass
+
+  try:
+      config = get_config()
+      verify(config)
+  except ConfigError as e:
+      print(e)
+      sys.exit(1)
+
+The ``get_config()`` function must convert the VyOS config to an abstract,
+internal representation. No other function is allowed to call the ``vyos.config.
+Config`` object method directly. The rationale for it is that when config reads
+are mixed with other logic, it's very hard to change the config syntax since
+you need to weed out every occurrence of the old syntax. If syntax-specific
+code is confined to a single function, the rest of the code can be left
+untouched as long as the internal representation remains compatible.
+
+Another advantage is testability of the code. Mocking the entire config
+subsystem is hard, while constructing an internal representation by hand is
+way simpler.
+
+The ``verify()`` function takes your internal representation of the config and
+checks if it's valid, otherwise it must raise ``ConfigError`` with an error
+message that describes the problem and possibly suggests how to fix it. It must
+not make any changes to the system. The rationale for it is again testability
+and, in the future when the config backend is ready and every script is
+rewritten in this fashion, ability to execute commit dry run ("commit test"
+like in JunOS) and abort commit before making any changes to the system if an
+error is found in any component.
+
+The ``generate()`` function generates config files for system components.
+
+The ``apply()`` function applies the generated configuration to the live
+system. It should use non-disruptive reload whenever possible. It may execute
+disruptive operations such as daemon process restart if a particular component
+does not support non-disruptive reload, or when the expected service degradation
+is minimal (for example, in case of auxiliary services such as LLDPd). In case
+of high impact services such as VPN daemon and routing protocols, when non-
+disruptive reload is supported for some but not all types of configuration
+changes, scripts authors should make effort to determine if a configuration
+change can be done in a non-disruptive way and only resort to disruptive restart
+if it cannot be avoided.
+
+Unless absolutely necessary, configuration scripts should not modify the active
+configuration of system components directly. Whenever at all possible, scripts
+should generate a configuration file or files that can be applied with a single
+command such as reloading a service through systemd init. Inserting statements
+one by one is particularly discouraged, for example, when configuring netfilter
+rules, saving them to a file and loading it with iptables-restore should always
+be preferred to executing iptables directly.
+
+The ``apply()`` and ``generate()`` functions may ``raise ConfigError`` if, for
+example, the daemon failed to start with the updated config. It shouldn't be a
+substitute for proper config checking in the ``verify()`` function. All
+reasonable effort should be made to verify that generated configuration is
+valid and will be accepted by the daemon, including, when necessary, cross-
+checks with other VyOS configuration subtrees.
+
+Exceptions, including ``VyOSError`` (which is raised by ``vyos.config.Config``
+on improper config operations, such as trying to use ``list_nodes()`` on a
+non-tag node) should not be silenced or caught and re-raised as config error.
+Sure this will not look pretty on user's screen, but it will make way better
+bug reports, and help users (and most VyOS users are IT professionals) do their
+own debugging as well.
+
+For easy orientation we suggest you take a look on the ``ntp.py`` or
+``interfaces-bonding.py`` (for tag nodes) implementation. Both files can be
+found in the vyos-1x_ repository.
+
+XML - CLI
+---------
+
+The bash (or better vbash) completion in VyOS is defined in *templates*.
+Templates are text files (called ``node.def``) stored in a directory tree. The
+directory names define the command names, and template files define the command
+behaviour. Before VyOS 1.2 (crux) this files were created by hand. After a
+complex redesign process_ the new style template are automatically generated
+from a XML input file.
+
+XML interface definitions for VyOS come with a RelaxNG schema and are located
+in the vyos-1x_ module. This schema is a slightly modified schema from VyConf_
+alias VyOS 2.0 So VyOS 1.2.x interface definitions will be reusable in Nextgen
+VyOS Versions with very minimal changes.
+
+The great thing about schemas is not only that people can know the complete
+grammar for certain, but also that it can be automatically verified. The
+`scripts/build-command-templates` script that converts the XML definitions to
+old style templates also verifies them against the schema, so a bad definition
+will cause the package build to fail. I do agree that the format is verbose, but
+there is no other format now that would allow this. Besides, a specialized XML
+editor can alleviate the issue with verbosity.
+
+Example:
+^^^^^^^^
+
+.. code-block:: xml
+
+  <?xml version="1.0"?>
+  <!-- Cron configuration -->
+  <interfaceDefinition>
+    <node name="system">
+      <children>
+        <node name="task-scheduler">
+          <properties>
+            <help>Task scheduler settings</help>
+          </properties>
+          <children>
+            <tagNode name="task" owner="${vyos_conf_scripts_dir}/task_scheduler.py">
+              <properties>
+                <help>Scheduled task</help>
+                <valueHelp>
+                  <format>&lt;string&gt;</format>
+                  <description>Task name</description>
+                </valueHelp>
+                <priority>999</priority>
+              </properties>
+              <children>
+                <leafNode name="crontab-spec">
+                  <properties>
+                    <help>UNIX crontab time specification string</help>
+                  </properties>
+                </leafNode>
+                <leafNode name="interval">
+                  <properties>
+                    <help>Execution interval</help>
+                    <valueHelp>
+                      <format>&lt;minutes&gt;</format>
+                      <description>Execution interval in minutes</description>
+                    </valueHelp>
+                    <valueHelp>
+                      <format>&lt;minutes&gt;m</format>
+                      <description>Execution interval in minutes</description>
+                    </valueHelp>
+                    <valueHelp>
+                      <format>&lt;hours&gt;h</format>
+                      <description>Execution interval in hours</description>
+                    </valueHelp>
+                    <valueHelp>
+                      <format>&lt;days&gt;d</format>
+                      <description>Execution interval in days</description>
+                    </valueHelp>
+                    <constraint>
+                      <regex>[1-9]([0-9]*)([mhd]{0,1})</regex>
+                    </constraint>
+                  </properties>
+                </leafNode>
+                <node name="executable">
+                  <properties>
+                    <help>Executable path and arguments</help>
+                  </properties>
+                  <children>
+                    <leafNode name="path">
+                      <properties>
+                        <help>Path to executable</help>
+                      </properties>
+                    </leafNode>
+                    <leafNode name="arguments">
+                      <properties>
+                        <help>Arguments passed to the executable</help>
+                      </properties>
+                    </leafNode>
+                  </children>
+                </node>
+              </children>
+            </tagNode>
+          </children>
+        </node>
+      </children>
+    </node>
+  </interfaceDefinition>
+
+Command definitions are purely declarative, and cannot contain any logic. All
+logic for generating config files for target applications, restarting services
+and so on is implemented in configuration scripts instead.
+
+Guidelines
+^^^^^^^^^^
+
+Use of numbers
+##############
+
+Use of numbers in command names **should** be avoided unless a number is a
+part of a protocol name or similar. Thus, ``protocols ospfv3`` is perfectly
+fine, but something like ``server-1`` is questionable at best.
+
+Help String
+###########
+
+To ensure uniform look and feel, and improve readability, we should follow a
+set of guidelines consistently.
+
+Capitalization and punctuation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The first word of every help string **must** be capitalized. There **must not**
+be a period at the end of help strings.
+
+Rationale: this seems to be the unwritten standard in network device CLIs, and
+a good aesthetic compromise.
+
+Examples:
+
+* Good: "Frobnication algorithm"
+* Bad: "frobnication algorithm"
+* Bad: "Frobnication algorithm."
+* Horrible: "frobnication algorithm."
+
+Use of abbreviations and acronyms
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Abbreviations and acronyms **must** be capitalized.
+
+Examples:
+
+* Good: "TCP connection timeout"
+* Bad: "tcp connection timeout"
+* Horrible: "Tcp connectin timeout"
+
+Acronyms also **must** be capitalized to visually distinguish them from normal
+words:
+
+Examples:
+
+* Good: RADIUS (as in remote authentication for dial-in user services)
+* Bad: radius (unless it's about the distance between a center of a circle and
+  any of its points)
+
+Some abbreviations are traditionally written in mixed case. Generally, if it
+contains words "over" or "version", the letter **should** be lowercase. If
+there's an accepted spelling (especially if defined by an RFC or another
+standard), it **must** be followed.
+
+Examples:
+
+* Good: PPPoE, IPsec
+* Bad: PPPOE, IPSEC
+* Bad: pppoe, ipsec
+
+Use of verbs
+~~~~~~~~~~~~
+
+Verbs **should** be avoided. If a verb can be omitted, omit it.
+
+Examples:
+
+* Good: "TCP connection timeout"
+* Bad: "Set TCP connection timeout"
+
+If a verb is essential, keep it. For example, in the help text of ``set system
+ipv6 disable-forwarding``, "Disable IPv6 forwarding on all interfaces" is a
+perfectly justified wording.
+
+Prefer infinitives
+~~~~~~~~~~~~~~~~~~
+
+Verbs, when they are necessary, **should** be in their infinitive form.
+
+Examples:
+
+* Good: "Disable IPv6 forwarding"
+* Bad: "Disables IPv6 forwarding"
+
+Migrating old CLI
+^^^^^^^^^^^^^^^^^
+
+.. list-table::
+   :widths: 25 25 50
+   :header-rows: 1
+
+   * - Old concept/syntax
+     - New syntax
+     - Notes
+   * - mynode/node.def
+     - <node name="mynode"> </node>
+     - Leaf nodes (nodes with values) use <leafNode> tag instead
+   * - mynode/node.tag , tag:
+     - <tagNode name="mynode> </node>
+     -
+   * - help: My node
+     - <properties> <help>My node</help>
+     -
+   * - val_help: <format>; some string
+     - <properties> <valueHelp> <format> format </format> <description> some
+       string </description>
+     - Do not add angle brackets around the format, they will be inserted
+       automatically
+   * - syntax:expression: pattern
+     - <properties> <constraint> <regex> ...
+     - <constraintErrorMessage> will be displayed on failure
+   * - syntax:expression: $VAR(@) in "foo", "bar", "baz"
+     - None
+     - Use regex
+   * - syntax:expression: exec ...
+     - <properties> <constraint> <validator> <name ="foo" argument="bar">
+     - "${vyos_libexecdir}/validators/foo bar $VAR(@)" will be executed,
+       <constraintErrorMessage> will be displayed on failure
+   * - syntax:expression: (arithmetic expression)
+     - None
+     - External arithmetic validator may be added if there's demand, complex
+       validation is better left to commit-time scripts
+   * - priority: 999
+     - <properties> <priority>999</priority>
+     - Please leave a comment explaining why the priority was chosen (e.g. "after
+       interfaces are configured")
+   * - multi:
+     - <properties> <multi/>
+     - Only applicable to leaf nodes
+   * - allowed: echo foo bar
+     - <properties> <completionHelp> <list> foo bar </list>
+     -
+   * - allowed: cli-shell-api listNodes vpn ipsec esp-group
+     - <properties> <completionHelp> <path> vpn ipsec esp-group </path> ...
+     -
+   * - allowed: /path/to/script
+     - <properties> <completionHelp> <script> /path/to/script </script> ...
+     -
+   * - default:
+     - None
+     - Move default values to scripts
+   * - commit:expression:
+     - None
+     - All commit time checks should be in the verify() function of the script
+   * - begin:/create:/delete:
+     - None
+     - All logic should be in the scripts
+
+.. _process: https://blog.vyos.io/vyos-development-digest-10
+.. _VyConf: https://github.com/vyos/vyconf/blob/master/data/schemata
+.. _vyos-1x: https://github.com/vyos/vyos-1x/blob/current/schema/
+.. _Jinja2: https://jinja.palletsprojects.com/
 .. _Phabricator: https://phabricator.vyos.net
\ No newline at end of file