CSIT-1135: Add bash code style guide document

- The document is currently an orphan.

Change-Id: I37377e2f7b5e928351336bd18ef037da4d3513fa
Signed-off-by: Vratko Polak <vrpolak@cisco.com>

2 files changed, 588 insertions, 1 deletions

diff --git a/docs/bash_code_style.rst b/docs/bash_code_style.rst
new file mode 100644
index 0000000000..9db968f7d8
--- /dev/null
+++ b/docs/bash_code_style.rst
@@ -0,0 +1,587 @@
+..
+   Copyright (c) 2019 Cisco and/or its affiliates.
+   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.
+
+
+The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
+"SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
+"MAY", and "OPTIONAL" in this document are to be interpreted as
+described in `BCP 14 <https://tools.ietf.org/html/bcp14>`_
+`[RFC2119] <https://tools.ietf.org/html/rfc2119>`_
+`[RFC8174] <https://tools.ietf.org/html/rfc8174>`_
+when, and only when, they appear in all capitals, as shown here.
+
+This document SHALL describe guidelines for writing reliable, maintainable,
+reusable and readable code for CSIT.
+
+Motivation
+^^^^^^^^^^
+
+TODO: List reasons why we need code style document for Bash.
+
+Proposed style
+^^^^^^^^^^^^^^
+
+File types
+~~~~~~~~~~
+
+Bash files SHOULD NOT be monolithic. Generally, this document
+considers two types of bash files:
+
++ Entry script: Assumed to be called by user,
+  or a script "external" in some way.
+
+  + Sources bash libraries and calls functions defined there.
+
++ Library file: To be sourced by entry scipts, possibly also by other libraries.
+
+  + Sources other libraries for functions it needs.
+
+    + Or relies on a related file already having sourced that.
+
+    + Documentation SHALL imply which case it is.
+
+  + Defines multiple functions other scripts can call.
+
+Safety
+~~~~~~
+
++ Variable expansions MUST be quoted, to prevent word splitting.
+
+  + This includes special "variables" such as "${1}".
+
+    + RECOMMENDED even if the value is safe, as in "$?" and "$#".
+
+  + It is RECOMMENDED to quote strings in general,
+    so text editors can syntax-highlight them.
+
+    + Even if the string is a numeric value.
+
+    + Commands and known options can get their own highlight, no need to quote.
+
+      + Example: You do not need to quote every word of
+        "pip install --upgrade virtualenv".
+
+  + Code SHALL NOT quote glob characters you need to expand (obviously).
+
+    + OPTIONALLY do not quote adjacent characters (such as dot or fore-slash),
+      so that syntax highlighting makes them stand out compared to surrounding
+      ordinary strings.
+
+    + Example: cp "logs"/*."log" "."/
+
+    + TODO: Consider giving examples both for good and bad usage.
+
+  + Command substitution on right hand side of assignment should be safe
+    without quotes.
+
+    + But still, quotes are RECOMMENDED, unless line length is a concern.
+
+    + Note that command substitution limits the scope for quotes,
+      so it is NOT REQUIRED to escape the quotes in deeper levels.
+
+    + TODO: Do we prefer backtics, or "dollar round-bracket"?
+
+      + Backticks are more readable, especially when there are
+        round brackets in the surrounding text.
+
+      + Backticks do not allow nested command substitution.
+
+        + But we might want to avoid nested command substitution anyway?
+
+  + Code SHOULD NOT be structured in a way where
+    word splitting is intended.
+
+    + Example: Variable holding string of multiple command lines arguments.
+
+    + Solution: Array variable should be used in this case.
+
+    + Expansion MUST use quotes then: "${name[@]}".
+
+    + Word splitting MAY be used when creating arrays from command substitution.
+
++ Code MUST always check the exit code of commands.
+
+  + Traditionally, error code checking is done either by "set -e"
+    or by appending "|| die" after each command.
+    The first is unreliable, due to many rules affecting "set -e" behavior
+    (see <https://mywiki.wooledge.org/BashFAQ/105>), but "|| die"
+    relies on humans identifying each command, which is also unreliable.
+    When was the last time you checked error code of "echo" command,
+    for example?
+
+  + As there is no reliable method of error detection, and there are two
+    largely independent unreliable methods, the best what we can do
+    is to apply both. So, code SHOULD explicitly
+    check each command (with "|| die" and similar) AND have "set -e" applied.
+
+  + Code MUST explicitly check each command, unless the command is well known,
+    and considered safe (such as the aforementioned "echo").
+
+    + The well known commands MUST still be checked implicitly via "set -e".
+
+  + See below for specific "set -e" recommendations.
+
++ Code SHOULD use "readlink -e" (or "-f" if target does not exist yet)
+  to normalize any path value to absolute path without symlinks.
+  It helps with debugging and identifies malformed paths.
+
++ Code SHOULD use such normalized paths for sourcing.
+
++ When exiting on a known error, code MUST print a longer, helpful message,
+  in order for the user to fix their situation if possible.
+
++ When error happens at an unexpected place, it is RECOMMENDED for the message
+  to be short and generic, instead of speculative.
+
+Bash options
+~~~~~~~~~~~~
+
++ Code MUST apply "-x" to make debugging easier.
+
+  + Code MAY temporarily supress such output in order to avoid spam
+    (e.g. in long busy loops), but it is still NOT RECOMMENDED to do so.
+
++ Code MUST apply "-e" for early error detection.
+
+  + But code still SHOULD use "|| die" for most commands,
+    as "-e" has numerous rules and exceptions.
+
+  + Code MAY apply "+e" temporarily for commands which (possibly nonzero)
+    exit code it interested in.
+
+    + Code MUST to store "$?" and call "set -e" immediatelly afterwards.
+
+  + Code MAY append "|| true" for benign commands,
+    when it is clear non-zero exit codes make no difference.
+
++ Code MUST apply "-u" as unset variable is generally a typo, thus an error.
+
+  + Code MAY temporarily apply "+u" if a command needs that to pass.
+
+    + Virtualenv activation is the only known example so far.
+
++ Code MUST apply "-o pipefail" to make sure "-e" picks errors
+  inside piped construct.
+
+  + Code MAY use "|| true" inside a pipe construct, in the (inprobable) case
+    when non-zero exit code still results in a meaningful pipe output.
+
++ All together: "set -exuo pipefail".
+
+  + Code MUST put that line near start of every file, so we are sure
+    the options are applied no matter what.
+
+    + "Near start" means "before any nontrivial code".
+
+    + Basically only copyright and long high-level comments are
+      RECOMMENDED to appear before.
+
+  + Also code MUST put the line near start of function bodies
+    and subshell invocations.
+
+Functions
+~~~~~~~~~
+
+There are (at least) two possibilities how a code from an external file
+can be executed. Either the file contains a code block to execute
+on each "source" invocation, or the file just defines functions
+which have to be called separately.
+
+This document considers the "function way" to be better,
+here are some pros and cons:
+
++ Cons:
+
+  + The function way takes more space. Files have more lines,
+    and the code in function body is one indent deeper.
+
+  + It is not easy to create functions for low-level argument manipulation,
+    as "shift" command in the function code does not affect the caller context.
+
+  + Call sites frequently refer to code two times,
+    when sourcing the definition and when executing the function.
+
+  + It is not clear when a library can rely on its relative
+    to have performed the sourcing already.
+
+  + Ideally, each library should detect if it has been sourced already
+    and return early, which takes even more space.
+
++ Pros:
+
+  + Some code blocks are more useful when used as function,
+    to make call site shorter.
+
+    + Examples: Trap functions, "die" function.
+
+  + The "import" part and "function" part usually have different side effects,
+    making the documentation more focused (even if longer overall).
+
+  + There is zero risk of argument-less invocation picking arguments
+    from parent context.
+
+    + This safety feature is the main reason for chosing the "function way".
+
+    + This allows code blocks to support optional arguments.
+
+TODO: Translate the "function way" into list of rules.
+
+Variables
+~~~~~~~~~
+
+This document describes two kinds of variables: called "local" and "global".
+
+TODO: Find better adjectives for the two kinds defined here,
+if the usual bash meaning makes reader forget other specifics.
+For example, variable with lowercase name and not marked by "local" builtin,
+is cosidered "global" from bash point of view, but "local" from this document
+point of view.
+
++ Local variables:
+
+  + Variable name MUST contain only lower case letters, digits and underscores.
+
+  + Code MUST NOT export local variables.
+
+  + Code MUST NOT rely on local variables set in different contexts.
+
+  + Documentation is NOT REQUIRED.
+
+    + Variable name SHOULD be descriptive enough.
+
+  + Local variable MUST be initialized before first use.
+
+    + Code SHOULD have a comment if a reader might have missed
+      the initialization.
+
+  + TODO: Agree on level of defensiveness (against local values
+    being influenced by other functions) needed.
+    Possible alternatives / additions to the "always initialize" rule:
+
+    + Unset local variables when leaving the function.
+
+    + Explicitly typeset by "local" builtin command.
+
+    + Require strict naming convention, e.g. function_name__variable_name.
+
++ Global variables:
+
+  + Variable name MUST contain only upper case letters, digits and underscores.
+
+  + They SHOULD NOT be exported, unless external commands need them
+    (e.g. PYTHONPATH).
+
+  + TODO: Propose a strict naming convention, or a central document
+    of all used global variables, to prevent contributors
+    from causing variable name conflicts.
+
+  + Code MUST document if a function (or its inner call)
+    reads a global variable.
+
+  + Code MUST document if a function (or its inner call)
+    sets or rewrites a global variable.
+
+  + If a function "wants to return a value", it SHOULD be implemented
+    as the function setting (or rewriting) a global variable,
+    and the call sites reading that variable.
+
+  + If a function "wants to accept an argument", it IS RECOMMENDED
+    to be implemented as the call sites setting or rewriting global variables,
+    and the function reading that variables.
+    But see below for direct arguments.
+
++ Code MUST use curly brackets when referencing variables,
+  e.g. "${my_variable}".
+
+  + It makes related constructs (such as ${name:-default}) less surprising.
+
+  + It looks more similar to Robot Framework variables (which is good).
+
+Arguments
+~~~~~~~~~
+
+Bash scripts and functions MAY accept arguments, named "${1}", "${2}" and so on.
+As a whole available via "$@".
+You MAY use "shift" command to consume an argument.
+
+Contexts
+````````
+
+Functions never have access to parent arguments, but they can read and write
+variables set or read by parent contexts.
+
+Arguments or variables
+``````````````````````
+
++ Both arguments and global variables MAY act as an input.
+
++ In general, if the caller is likely to supply the value already placed
+  in a global variable of known name, it is RECOMMENDED
+  to use that global variable.
+
++ Construct "${NAME:-value}" can be used equally well for arguments,
+  so default values are possible for both input methods.
+
++ Arguments are positional, so there are restrictions on which input
+  is optional.
+
++ Functions SHOULD either look at arguments (possibly also
+  reading global variables to use as defaults), or look at variables only.
+
++ Code MUST NOT rely on "${0}", it SHOULD use "${BASH_SOURCE[0]}" instead
+  (and apply "readlink -e") to get the current block location.
+
++ For entry scripts, it is RECOMMENDED to use standard parsing capabilities.
+
+  + For most Linux distros, "getopt" is RECOMMENDED.
+
+Function size
+~~~~~~~~~~~~~
+
++ In general, code SHOULD follow reasoning similar to how pylint
+  limits code complexity.
+
++ It is RECOMMENDED to have functions somewhat simpler than Python functions,
+  as Bash is generally more verbose and less readable.
+
++ If code contains comments in order to partition a block
+  into sub-blocks, the sub-blocks SHOULD be moved into separate functions.
+
+  + Unless the sub-blocks are essentially one-liners,
+    not readable just because external commands do not have
+    obvious enough parameters. Use common sense.
+
+Documentation
+~~~~~~~~~~~~~
+
++ The library path and filename is visible from source sites. It SHOULD be
+  descriptive enough, so reader do not need to look inside to determine
+  how and why is the sourced file used.
+
+  + If code would use several functions with similar names,
+    it is RECOMMENDED to create a (well-named) sub-library for them.
+
+  + Code MAY create deep library trees if needed, it SHOULD store
+    common path prefixes into global variables to make sourcing easier.
+
+  + Contributors, look at other files in the subdirectory. You SHOULD
+    improve their filenames when adding-removing other filenames.
+
+  + Library files SHOULD NOT have executable flag set.
+
+  + Library files SHOULD have an extension .sh (or perhaps .bash).
+
+  + It is RECOMMENDED for entry scripts to also have executable flag unset
+    and have .sh extension.
+
++ Each entry script MUST start with a shebang.
+
+  + "#!/bin/usr/env bash" is RECOMMENDED.
+
+  + Code SHOULD put an empty line after shebang.
+
+  + Library files SHOULD NOT contain a shebang, as "source" is the primary
+    method to include them.
+
++ Following that, there SHOULD be a block of comment lines with copyright.
+
+  + It is a boilerplate, but human eyes are good at ignoring it.
+
+  + Overhead for git is also negligible.
+
++ Following that, there MUST be "set -exuo pipefail".
+
+  + It acts as an anchor for humans to start paying attention.
+
+Then it depends on script type.
+
+Library documentation
+`````````````````````
+
++ Following "set -exuo pipefail" SHALL come the "import part" documentation.
+
++ Then SHALL be the import code
+  ("source" commands and a bare minimum they need).
+
++ Then SHALL be the function definitions, and inside:
+
+  + "set -exuo pipefail" again.
+
+  + Following that SHALL be the function documentation explaining API contract.
+    Similar to Robot [Documentation] or Python function-level docstring.
+
+    + See below.
+
+  + Following that SHALL be varius TODOs, FIXMEs and code itself.
+
+    + "Code itself" SHALL include comment lines
+      explaining any non-obvious logic.
+
+    + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs.
+
+  + There SHALL be two empty lines before next function definition.
+
+More details on function documentation:
+
+Generally, code SHOULD use comments to explain anything
+not obvious from the funtion name.
+
++ Function documentation SHOULD start with short description of function
+  operation or motivation, but only if not obvious from function name.
+
++ Documentation SHOULD continue with listing any non-obvious side effect:
+
+  + Documentation MUST list all read global variables.
+
+    + Documentation SHOULD include descriptions of semantics
+      of global variable values.
+      It is RECOMMENDED to mention which function is supposed to set them.
+
+    + The "include descriptions" part SHOULD apply to other items as well.
+
+  + Documentation MUST list all global variables set, unset, reset,
+    or otherwise updated.
+
+  + It is RECOMMENDED to list all hardcoded values used in code.
+
+    + Not critical, but can hint at future improvements.
+
+  + Documentation MUST list all files or directories read
+    (so caller can make sure their content is ready).
+
+  + Documentation MUST list all files or directories updated
+    (created, deleted, emptied, otherwise edited).
+
+  + Documentation SHOULD list all functions called (so reader can look them up).
+
+    + Documentation SHOULD mention where are the functions defined,
+      if not in the current file.
+
+  + Documentation SHOULD list all external commands executed.
+
+    + Because their behavior can change "out of bounds", meaning
+      the contributor changing the implementation of the extrenal command
+      can be unaware of this particular function interested in its side effects.
+
+  + Documentation SHOULD explain exit code (coming from
+    the last executed command).
+
+    + Usually, most functions SHOULD be "pass or die",
+      but some callers MAY be interested in nonzero exit codes
+      without using global variables to store them.
+
+    + Remember, "exit 1" ends not only the function, but all scripts
+      in the source chain, so code MUST NOT use it for other purposes.
+
+      + Code SHOULD call "die" function instead. This way the caller can
+        redefine that function, if there is a good reason for not exiting
+        on function failure.
+
+  + TODO: Programs installed, services started, URLs downloaded from, ...
+
+  + TODO: Add more items when you spot them.
+
+  + TODO: Is the current order recommended?
+
+Entry script documentation
+``````````````````````````
+
++ After "set -exuo pipefail", high-level description SHALL come.
+
+  + Then TODOs and FIXMEs SHALL be placed (if any).
+
+  + Entry scripts are rarely reused, so detailed side effects
+    are OPTIONAL to document.
+
+  + But code SHOULD document the primary side effects.
+
++ Then SHALL come few commented lines to import the library with "die" function.
+
++ Then block of "source" commands for sourcing other libraries needed SHALL be.
+
+  + In alphabetical order, any "special" library SHOULD be
+    in the previous block (for "die").
+
++ Then block os commands processing arguments SHOULD be (if needed).
+
++ Then SHALL come block of function calls (with parameters as needed).
+
+Other general recommendations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
++ Code SHOULD NOT not repeat itself, even in documentation:
+
+  + For hardcoded values, a general description SHOULD be written
+    (instead of copying the value), so when someone edits the value
+    in the code, the description still applies.
+
+  + If affected directory name is taken from a global variable,
+    documentation MAY distribute the directory description
+    over the two items.
+
+  + If most of side effects come from an inner call,
+    documentation MAY point the reader to the documentation
+    of the called function (instead of listing all the side effects).
+
+    + TODO: Composite functions can have large effects. Should we require
+      intermediate functions to actively hide them whenever possible?
+
++ But documentation SHOULD repeat it if the information crosses functions.
+
+  + Item description MUST NOT be skipped just because the reader
+    should have read parent/child documentation already.
+
+  + Frequently it is RECOMMENDED to copy&paste item descriptions
+    between functions.
+
+  + But sometimes it is RECOMMENDED to vary the descriptions. For example:
+
+    + A global variable setter MAY document how does it figure out the value
+      (without caring about what it will be used for by other functions).
+
+    + A global variable reader MAY document how does it use the value
+      (without caring about how has it been figured out by the setter).
+
++ When possible, Bash code SHOULD be made to look like Python
+  (or Robot Framework). Those are three primary languages CSIT code relies on,
+  so it is nicer for the readers to see similar expressions when possible.
+  Examples:
+
+  + Code MUST use indentation, 1 level is 4 spaces.
+
+  + Code SHOULD use "if" instead of "&&" constructs.
+
+  + For comparisons, code SHOULD use operators such as "!=" (needs "[[").
+
++ Code MUST NOT use more than 80 characters per line.
+
+  + If long external command invocations are needed,
+    code SHOULD use array variables to shorten them.
+
+  + If long strings (or arrays) are needed, code SHOULD use "+=" operator
+    to grow the value over multiple lines.
+
+  + If "|| die" does not fit with the command, code SHOULD use curly braces:
+
+    + Current line has "|| {",
+
+    + Next line has the die commands (indented one level deeper),
+
+    + Final line closes with "}" at original intent level.
+
+  + TODO: Recommend what to do with other constructs.
+
+    + For example multiple piped commands.
+
+    + No, "eval" is too unsafe to use.
diff --git a/docs/test_code_guidelines.rst b/docs/test_code_guidelines.rst
index 51a99f077a..b455c924c4 100644
--- a/docs/test_code_guidelines.rst
+++ b/docs/test_code_guidelines.rst
@@ -319,4 +319,4 @@ and unrelated ones such as PAL) if there are any (in addition to library ones).
 Bash scripts and libraries
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-TODO: Link here when document for this is ready.
+TODO: Link or copy the bash_code_style.rst document here.