Merge branch 'main' into advanced_config_tutorial

.github/workflows/checkbox-beta-release.yml

@@ -2,10 +2,34 @@ name: Beta version of checkbox
 run-name: Promote edge versions of checkbox to beta
 
 on:
+  push:
+    branches:
+      - beta
   workflow_dispatch:
 
 jobs:
+  should-run:
+    runs-on: [self-hosted, linux, large]
+      - name: Setup the gh repository and install gh
+        run: |
+          which curl || (sudo apt update && sudo apt install curl -y)
+          sudo curl https://cli.github.com/packages/githubcli-archive-keyring.gpg --output /usr/share/keyrings/githubcli-archive-keyring.gpg
+          sudo chmod go+r /usr/share/keyrings/githubcli-archive-keyring.gpg
+          gpg --import /usr/share/keyrings/githubcli-archive-keyring.gpg
+          gpg --fingerprint "2C6106201985B60E6C7AC87323F3D4EA75716059"
+          echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" | sudo tee /etc/apt/sources.list.d/github-cli.list > /dev/null
+          sudo apt update -qq
+          sudo apt install -qq -y gh
+      - name: Checkout checkbox monorepo
+        uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+      - name: Verify Promotion Conditions
+        run: |
+          tools/release/can_promote_edge.py
+
   release-notes:
+    needs: should-run
     runs-on: [self-hosted, linux, large]
     steps:
       - name: Checkout checkbox monorepo
@@ -31,6 +55,7 @@ jobs:
           gh release create $(git describe --tags --abbrev=0 --match v*) -d --generate-notes
 
   checkbox_deb_packages:
+    needs: should-run
     name: Checkbox Debian packages
     runs-on: [self-hosted, linux, large]
     steps:
@@ -49,6 +74,7 @@ jobs:
           tools/release/lp_copy_packages.py checkbox-dev edge checkbox-dev beta
 
   checkbox_core_snap:
+    needs: should-run
     name: Checkbox core snap packages
     runs-on: [self-hosted, linux, large]
     env:
@@ -70,6 +96,7 @@ jobs:
           yes | snapcraft promote checkbox22 --from-channel latest/edge --to-channel latest/beta
 
   checkbox_snap:
+    needs: should-run
     name: Checkbox snap packages
     runs-on: [self-hosted, linux, large]
     env:

.github/workflows/checkbox-snap-daily-builds.yml

@@ -7,6 +7,7 @@ on:
 jobs:
   snap:
     strategy:
+      fail-fast: false
       matrix:
         type: [classic, uc]
         releases: [16, 18, 20, 22]

.vscode/settings.json

@@ -0,0 +1,5 @@
+{
+    "[markdown]": {
+        "editor.formatOnSave": false
+    }
+}

CONTRIBUTING.md

@@ -302,12 +302,53 @@ about:
 
 1. [Push your changes] to your GitHub repository.
 
-### Finally...
+### Code review criteria & workflow
 
-Once enough people have reviewed and approved your work, it can be merged into
-the main branch of the main repository. Ask a member of the Checkbox team to do
-this. The branch should be then shortly automatically merged. The pull request
-status will then switch to “Merged”.
+As a general rule there is one reviewer per PR. Other reviewers may pop in to smooth up the process (where they feel confident they can _approve and merge_ changes after some were requested by another review). When more reviewers are needed an approval of, comments explaining that are made in the comment thread. Chiefly this is done to work around cases of low test coverage or when the changes affect something known to be of low quality (i.e. something significantly complex and hard to reason about, brittle, dated, known to have broken in the past, etc).
+
+On accepting a change without any change requests, the approving reviewer will merge the pull request. If they choose not to do that, they leave a comment explaining the rationale (mostly this exception is to cover situations when significant changes need to be staged across multiple releases).
+
+The reviewer is encouraged to use suggestions to communicate exact intended solutions, and to make it easy to apply them. The reviewer _must_ do this when making trivial style related suggestions. The reviewer might also post code into the PR, or to a branch branched off from the feature branch, to communicate more complex suggestions. 
+
+At all cases when a Checkbox maintainer picks up a PR for review, they are prepared to go back to the same PR if needed (if changes were requested).
+
+Checkbox maintainers will follow the following process to make the determination between "accept", "request changes" and "comment".
+
+#### Reviewer will approve the pull request when...
+
+1. They have read and understood the pull request description and changes being proposed.
+2. The requirements laid out [in the PR template](.github/pull_request_template.md) are met. In particular:
+  - the reviewer is convinced the changed code works as advertised.
+  - tests introduced cover the new functionality, as well as untouched code it may affect.
+  - testing reported by the author covers the new functionality, as well as untouched code it may affect. 
+  - if needed, reviewer has tested the changed solution locally.
+
+If the PR has no problems to address that requires actions from its contributor, reviewer merges it upon approval (and deletes the feature branch).
+
+When non-blocking issues are encountered by the reviewer, they mark the PR "approved" and leave the changes and resulting merge to the contributor. Approval with comments is done by the reviewer when:
+
+- Reviewer's change requests on the PR are only at most subjective (taste is a factor, e.g. arguable readability benefits).
+- Reviewer is proposing cleaner / simpler alternatives to some parts of the code being proposed that if un-addressed is not risky to address in follow-up.
+- Reviewer is proposing changes that are best (safer, faster, more productive, etc) to address in follow-up PRs. The reviewer will in these cases file follow-up issues, and link to them _in the PR description_.
+- The code was determined to be difficult to read, i.e. it _could_ be improved, but in the end the reviewer understood it (reviewer leaves a comment in these cases, but leaves it to contributor discretion to address).
+- Reviewer believes they have not caught a problem, but asks question(s) out of curiosity or to confirm their understanding (on contributor then to consider the question and either make further changes or simply answer it).
+- Reviewer points out issues unrelated to what the PR is trying to accomplish (i.e. the problem existed before).
+
+#### Reviewer will "request changes" to your PR when...
+
+1. the pull request description is unclear, or it is clear that the changes do not meet the requirements [in the PR template](.github/pull_request_template.md).
+2. doesn't do what is claimed.
+3. introduces a new bug.
+4. introduces a maintenance problem.
+5. the solution is unnecessarily, significantly, too complex for the problem being solved.
+6. the introduced code / patch is very difficult to understand (the reviewer has doubts of understanding it right, or doubts that others would).
+7. the PR should be split into multiple parts (is too big to safely review, i.e. may hide critical issues). This call is not to be done for sake of readability, it is done for safety: 
+  - if reviewer believes it were _more elegant_ to split the PR, they should approve and comment
+  - if the reviewer believes to not be doing a good job reviewing it without it being split, they should "request changes" and in their comment request for it to be split.
+
+#### Reviewer will "comment" if...
+
+They are not confident in making a call, delegating explicitly in a comment to a reviewer who they believe _can_ make a call, as quickly and as early as possible in the process.
 
 ## Documentation
 

checkbox-ng/MANIFEST.in

@@ -28,9 +28,11 @@ include plainbox/impl/providers/manifest/po/*.po
 include plainbox/impl/providers/manifest/po/*.pot
 include plainbox/impl/providers/manifest/po/POTFILES.in
 include plainbox/impl/providers/manifest/units/*.pxu
+include plainbox/vendor/inxi
 
 recursive-exclude daily-package-testing *
 recursive-include contrib *.policy
 recursive-include docs *.rst *.py *.html *.conf
 recursive-include plainbox/test-data *.json *.html *.tar.xz
 recursive-exclude debian *
+recursive-exclude build *

checkbox-ng/plainbox/impl/providers/exporters/data/checkbox.json

@@ -3,6 +3,7 @@
 {%- set resource_map = state.resource_map -%}
 {%- set job_state_map = state.job_state_map -%}
 {%- set category_map = state.category_map -%}
+{%- set system_information = state.system_information -%}
 {
     "title": {{ state.metadata.title | jsonify | safe }},
 {%- if "testplan_id" in app_blob %}
@@ -115,7 +116,8 @@
         {%- for cat_id, cat_name in category_map|dictsort %}
         "{{ cat_id }}": "{{ cat_name }}"{%- if not loop.last -%},{%- endif %}
         {%- endfor %}
-    }
+    },
+    "system_information": {{system_information.to_json() | safe}}
 {%- if ns ~ 'dkms_info_json' in state.job_state_map and state.job_state_map[ns ~ 'dkms_info_json'].result.outcome == 'pass' %},
 {%- set dkms_info_json = '{' + state.job_state_map[ns ~ 'dkms_info_json'].result.io_log_as_text_attachment.split('{', 1)[-1] %}
     "dkms_info": {{ dkms_info_json | indent(4, false) | safe }}

checkbox-ng/plainbox/impl/session/README.md

@@ -0,0 +1,79 @@
+# System Information Collection
+
+Checkbox collects various information about the system using Collectors.
+A Collector is a class that wraps a tool that can provide a JSON-serializable
+set of information.
+When Checkbox starts, it uses these collectors to collect and store
+in the session storage all the information it can gather. This is done before
+the first session checkpoint is created. From then onward, these information
+will remain in memory (and on disk in the checkpoint). When generating a
+submission report, Checkbox will include all the information in a top-level
+field of the json called "system_information".
+
+## Format
+
+A collector can either run succesfully or fail. Regardless of the result,
+running a collector will create a new field in the submission file following
+this format:
+
+```
+"system_information" : {
+  collector_name : {
+    "version" : collector_version,
+    "success" : true/false,
+    "outputs" : { ... }
+}
+```
+
+The outputs field's format depends on the success of the collection.
+
+If it ran successfully, the output field will have the follwing structure:
+
+```
+  "outputs" : {
+    "payload" : collector_parsed_json_output,
+    "stderr" : collector_error_log
+  }
+```
+Where the `collector.payload` is either an array or a dictionary.
+
+If it failed to run, the output field will have the following structure:
+
+```
+  "outputs" : {
+    "stdout" : collector_output_log,
+    "stderr" : collector_error_log
+  }
+```
+Where `collector_error_log` and `collector_output_log` are a string.
+
+## Creating new collectors
+
+To create a new collector, one has to create a class that uses the
+`CollectorMeta` metaclass. Additionally every collector has to define
+a `COLLECTOR_NAME`. Refer to the docstring of `CollectorMeta` for a more
+in-dept description.
+
+> Note: Before creating a new collector, verify if the functionality that
+> is needed is already implemented in an existing `collector`. If so, always
+> prefer using an already existing collector than creating a new one
+
+
+### Using external tools
+
+If the collector needs a tool, it should be added, when appropriate, to the
+vendorized section of Checkbox. Vendorization refers to the inclusion of
+external resource to a project's codebase.
+
+To vendorize a tool, locate the `vendor` module within Checkbox and place
+the vendorized version of the tool there, add to `vendor/__init__.py`
+the path to the executable that you have added.
+
+**It is appropriate** to add a vendorized tool when it is an executable script
+interpreted by any interpreter pre-installed on **every** version of Ubuntu that
+Checkbox supports. The tool must have a compatible license with the Checkbox
+license (GPLv3).
+
+**It is not appropriate** to add a compiled tool of any kind. Since Checkbox
+is designed to run on various architectures, compiled tools might not be
+universally compatible, leading to operational issues.

checkbox-ng/plainbox/impl/session/resume.py

@@ -63,6 +63,7 @@
 from plainbox.impl.secure.qualifiers import SimpleQualifier
 from plainbox.impl.session.state import SessionMetaData
 from plainbox.impl.session.state import SessionState
+from plainbox.impl.session.system_information import CollectionOutput, CollectorOutputs
 
 logger = logging.getLogger("plainbox.session.resume")
 
@@ -202,6 +203,8 @@ def _peek_json(self, json_repr):
             return SessionPeekHelper6().peek_json(json_repr)
         elif version == 7:
             return SessionPeekHelper7().peek_json(json_repr)
+        elif version == 8:
+            return SessionPeekHelper8().peek_json(json_repr)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -333,6 +336,9 @@ def _resume_json(self, json_repr, early_cb=None):
         elif version == 7:
             helper = SessionResumeHelper7(
                 self.job_list, self.flags, self.location)
+        elif version == 8:
+            helper = SessionResumeHelper8(
+                self.job_list, self.flags, self.location)
         else:
             raise IncompatibleSessionError(
                 _("Unsupported version {}").format(version))
@@ -586,6 +592,18 @@ class SessionPeekHelper7(MetaDataHelper7MixIn, SessionPeekHelper6):
     The only goal of this class is to reconstruct session state meta-data.
     """
 
+class SessionPeekHelper8(MetaDataHelper7MixIn, SessionPeekHelper6):
+    """
+    Helper class for implementing session peek feature
+
+    This class works with data constructed by
+    :class:`~plainbox.impl.session.suspend.SessionSuspendHelper7` which has
+    been pre-processed by :class:`SessionPeekHelper` (to strip the initial
+    envelope).
+
+    The only goal of this class is to reconstruct session state meta-data.
+    """
+
 class SessionResumeHelper1(MetaDataHelper1MixIn):
 
     """
@@ -1157,10 +1175,28 @@ def _build_SessionState(self, session_repr, early_cb=None):
         logger.debug(_("Resume complete!"))
         return session
 
-
 class SessionResumeHelper7(MetaDataHelper7MixIn, SessionResumeHelper6):
     pass
 
+class SessionResumeHelper8(SessionResumeHelper7):
+    def _restore_SessionState_system_information(self, session_state, session_repr):
+        _validate(session_repr, key="system_information", value_type=dict)
+        system_information = CollectorOutputs(
+            {
+                tool_name: CollectionOutput.from_dict(tool_output_json)
+                for (tool_name, tool_output_json) in session_repr[
+                    "system_information"
+                ].items()
+            }
+        )
+        session_state.system_information = system_information
+
+    def _build_SessionState(self, session_repr, early_cb=None):
+        session_state = super()._build_SessionState(session_repr, early_cb)
+        self._restore_SessionState_system_information(
+            session_state, session_repr
+        )
+        return session_state
 
 def _validate(obj, **flags):
     """Multi-purpose extraction and validation function."""

checkbox-ng/plainbox/impl/session/state.py

@@ -35,6 +35,9 @@
 from plainbox.impl.secure.qualifiers import select_jobs
 from plainbox.impl.session.jobs import JobState
 from plainbox.impl.session.jobs import UndesiredJobReadinessInhibitor
+from plainbox.impl.session.system_information import(
+    collect as collect_system_information
+)
 from plainbox.impl.unit.job import JobDefinition
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.testplan import TestPlanUnitSupport
@@ -746,6 +749,9 @@ def __init__(self, unit_list):
         self._resource_map = {}
         self._fake_resources = False
         self._metadata = SessionMetaData()
+        # If unset, this is loaded via system_information
+        self._system_information = None
+
         super(SessionState, self).__init__()
 
     def trim_job_list(self, qualifier):
@@ -815,6 +821,18 @@ def trim_job_list(self, qualifier):
                 self.on_job_removed(job)
                 self.on_unit_removed(job)
 
+    @property
+    def system_information(self):
+        if not self._system_information:
+            # This is a new session, we need to query this infos
+            self._system_information = collect_system_information()
+        return self._system_information
+
+    @system_information.setter
+    def system_information(self, value):
+        #TODO: check if system_information was already set
+        self._system_information = value
+
     def update_mandatory_job_list(self, mandatory_job_list):
         """
         Update the set of mandatory jobs (that must run).

checkbox-ng/plainbox/impl/session/suspend.py

@@ -661,6 +661,16 @@ def _repr_SessionMetaData(self, obj, session_dir):
         data['last_job_start_time'] = obj.last_job_start_time
         return data
 
+class SessionSuspendHelper8(SessionSuspendHelper7):
+    VERSION = 8
+
+    def _repr_SessionState(self, obj, session_dir):
+        data = super()._repr_SessionState(obj, session_dir)
+        data["system_information"] = {
+            tool_name: tool_output.to_dict()
+            for (tool_name, tool_output) in obj.system_information.items()
+        }
+        return data
 
 # Alias for the most recent version
-SessionSuspendHelper = SessionSuspendHelper7
+SessionSuspendHelper = SessionSuspendHelper8