doc: document runcmd exit code semantics and error handling - #7041
doc: document runcmd exit code semantics and error handling#7041sundeep8967 wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
documentation
|
bootcmd has the same behaviour, maybe we should mention this? |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
@sundeep8967 thank you for the PR. Let's please tighten up that documentation to make it a bit more concise. Too many long notes make it a bit more challenging to read.
Also @nabil-rady thanks for the point, but I disagree slightly with including this same comment in cc_bootcmd/data.yaml because cc_bootcmd/data.yaml attempts to cross-reference cc_runcmd.
But, what I think would make the runcmd reference more discoverable would be to change the cc_bootcmd/data.yaml description to actually create an HTML anchor to runcmd via an official reference to allow readers to click through to obtain the runcmd reference more easily.
--- a/doc/module-docs/cc_bootcmd/data.yaml
+++ b/doc/module-docs/cc_bootcmd/data.yaml
@@ -4,7 +4,7 @@ cc_bootcmd:
slightly after a boothook would run. This is very similar to a boothook,
but more user friendly. Commands can be
specified either as lists or strings. For invocation details, see
- ``runcmd``.
+ :ref:`runcmd<mod_cc_runcmd>`.| exit status of the last executed command by default. To stop execution and | ||
| fail immediately on any command error, include ``set -e`` as the first | ||
| command or handle command exit codes explicitly. | ||
|
|
There was a problem hiding this comment.
Thank you for the contribution , let's try to avoid use of leading subordinating conjunctions such as "Because .... ," as that just adds indirection and reading complexity for the reader. Can we make this more succint?
Maybe something like:
By default, the runcmd exit status is that of the last command. Add a set -e as the first command to abort on first error.
If you have other suggestions that would be great.
There was a problem hiding this comment.
No, I don't think we should include the comment about set -e. set -e is notoriously error prone.
| :ref:`Final boot stage <boot-Final>`. | ||
|
|
||
| .. note:: | ||
| Because commands are written line-by-line to a shell script executed by |
There was a problem hiding this comment.
Please don't document implementation details.
| fail immediately on any command error, include ``set -e`` as the first | ||
| command or handle command exit codes explicitly. |
There was a problem hiding this comment.
set -e doesn't always work - please don't document it like this.
| that actually runs the script is ``scripts_user`` in the | ||
| :ref:`Final boot stage <boot-Final>`. | ||
|
|
||
| .. note:: |
There was a problem hiding this comment.
I don't think that having a bunch of "note" sections makes sense. Exit behavior probably deserves to be in the main section.
There was a problem hiding this comment.
I agree with your suggestion of including just the exit behavior in the main section. Dropping the set -e suggestion/workaround makes that content a little more appropriate to include in the header.
|
thanks for the review! updated the docs in cc_bootcmd to use the official sphinx ref |
|
Just missing the other comments brett made and we'll take another look. |
Clarify in the cc_runcmd reference documentation that commands in the runcmd module are written to a shell script executed by /bin/sh, and therefore the overall exit status of the runcmd step is determined by the exit code of the last executed command. Document that users desiring fail-fast behavior across all commands should include 'set -e' as the first command or explicitly handle exit codes. Fixes canonicalGH-7037 Signed-off-by: sundeep8967 <sundeep8967@gmail.com>
Update the cc_bootcmd documentation to use official Sphinx :ref:`runcmd<mod_cc_runcmd>` reference rather than literal backticks for improved cross-linking. Signed-off-by: sundeep8967 <sundeep8967@gmail.com>
2abae8f to
a2ec99d
Compare
Signed-off-by: sundeep8967 <sundeep8967@gmail.com>
|
Thanks @holmanb and @blackboxsw! Updated as suggested: moved the exit code behavior directly into the main description body of |
4 participants
Problem
When using
runcmdwith multiple commands, users observed that the exit code of the final command in the list determines whethercloud-initreports success or failure (e.g. earlier failed commands are ignored if the final command returns 0). This behavior was not explicitly documented in the module reference.Solution
doc/module-docs/cc_runcmd/data.yamldocumenting thatruncmdscripts are executed by/bin/shline-by-line, and the overall exit code matches the exit code of the last executed command by default.set -eas the first command or explicitly handle exit status.Fixes #7037
Testing
pytest tests/unittests/config/test_schema.py tests/unittests/config/test_cc_runcmd.py(164 passed, 2 skipped).