[CERTTF-424] feat: support a reference directory for attachments (#374)

* feat(cli): introduce support for attachments reference directory
* feat: attachment reference directory support in the submit Github action
* feat: change default behaviour to consider attachments relative to job file
* chore: add tests for packing attachments (include using a reference)
* docs: add documentation for new command-line argument
* docs: update documentation to reflect change in default attachment behaviour

.github/actions/submit/action.yaml

@@ -19,6 +19,9 @@ inputs:
     description: The Testflinger server to use
     required: false
     default: testflinger.canonical.com
+  attachments-relative-to:
+    description: The reference directory for relative attachment paths
+    required: false
 outputs:
   id:
     description: 'The ID of the submitted job'
@@ -81,8 +84,12 @@ runs:
       shell: bash
       env:
         SERVER: https://${{ inputs.server }}
+        RELATIVE_TO: ${{ inputs.attachments-relative-to }}
       run: |
-        JOB_ID=$(testflinger --server $SERVER submit --quiet "$JOB")
+        if [ -n "$RELATIVE_TO" ]; then
+          RELATIVE="--attachments-relative-to $RELATIVE_TO"
+        fi
+        JOB_ID=$(testflinger --server $SERVER submit --quiet "$RELATIVE" "$JOB")
         echo "job id: $JOB_ID"
         echo "id=$JOB_ID" >> $GITHUB_OUTPUT
 

README.md

@@ -39,7 +39,7 @@ This monorepo is organized in a way that is consistant with the components descr
 
 # Github actions
 
-If you need to submit a job to a testflinger server through a Github action (instead, for example, of using the command-line tool), you can use the [`submit` action](https://github.com/canonical/testflinger/blob/main/.github/actions/submit/action.yaml) in a CI workflow.
+If you need to submit a job to a testflinger server through a Github action (instead, for example, of using the command-line tool), you can use the [`submit` action](https://github.com/canonical/testflinger/blob/main/.github/actions/submit/action.yaml) in a CI workflow. Please refer to the `inputs` field of the action for a complete list of the arguments that the action can receive.
 
 The corresponding step in the workflow would look like this:
 ```yaml

cli/testflinger_cli/__init__.py

@@ -125,15 +125,6 @@ def _print_queue_message():
     )
 
 
-def make_relative(path: Path) -> Path:
-    """Return resolved relative `path`"""
-    if path.is_absolute():
-        # strip leading '/' from absolute path
-        path = path.relative_to(path.root)
-    # resolve the path and make relative again
-    return path.resolve().relative_to(Path.cwd())
-
-
 class AttachmentError(Exception):
     """Exception thrown when attachments fail to be submitted"""
 
@@ -292,6 +283,12 @@ def get_args(self):
         arg_submit.add_argument("--poll", "-p", action="store_true")
         arg_submit.add_argument("--quiet", "-q", action="store_true")
         arg_submit.add_argument("filename")
+        relative = arg_submit.add_mutually_exclusive_group()
+        relative.add_argument(
+            "--attachments-relative-to",
+            dest="relative",
+            help="The reference directory for relative attachment paths",
+        )
 
         self.args = parser.parse_args()
         self.help = parser.format_help()
@@ -347,17 +344,19 @@ def configure(self):
     @staticmethod
     def extract_attachment_data(job_data: dict) -> Optional[dict]:
         """Pull together the attachment data per phase from the `job_data`"""
-        attachment_data = {}
+        attachments = {}
         for phase in ("provision", "firmware_update", "test"):
-            phase_str = f"{phase}_data"
             try:
-                attachment_data[phase] = job_data[phase_str]["attachments"]
+                attachments[phase] = [
+                    attachment
+                    for attachment in job_data[f"{phase}_data"]["attachments"]
+                    if attachment.get("local")
+                ]
             except KeyError:
                 pass
-        return attachment_data or None
+        return attachments or None
 
-    @staticmethod
-    def pack_attachments(archive: str, attachment_data: dict):
+    def pack_attachments(self, archive: str, attachment_data: dict):
         """Pack the attachments specifed by `attachment_data` into `archive`
 
         Use `tarfile` instead of `shutil` because:
@@ -367,15 +366,42 @@ def pack_attachments(archive: str, attachment_data: dict):
         > owner.
         Ref: https://docs.python.org/3/library/tarfile.html
         """
+        # determine the reference directory for relative attachment paths
+        if self.args.relative:
+            # provided as a command-line argument
+            reference = Path(self.args.relative).resolve(strict=True)
+        else:
+            # retrieved from the directory where the job file is contained
+            reference = Path(self.args.filename).parent.resolve(strict=True)
+
         with tarfile.open(archive, "w:gz") as tar:
             for phase, attachments in attachment_data.items():
                 phase_path = Path(phase)
                 for attachment in attachments:
                     local_path = Path(attachment["local"])
-                    # determine archive name for attachment
+                    if not local_path.is_absolute():
+                        # make relative attachment path absolute
+                        local_path = reference / local_path
+                    local_path = local_path.resolve()
+                    # determine the archive path for the attachment
                     # (essentially: the destination path on the agent host)
-                    agent_path = Path(attachment.get("agent", local_path))
-                    agent_path = make_relative(agent_path)
+                    try:
+                        agent_path = Path(attachment["agent"])
+                        if agent_path.is_absolute():
+                            # strip leading '/' from absolute path
+                            agent_path = agent_path.relative_to(
+                                agent_path.root
+                            )
+                    except KeyError:
+                        # no agent path provided: determine it from local path
+                        try:
+                            # make agent path relative to the reference path
+                            agent_path = local_path.relative_to(reference)
+                        except ValueError:
+                            # unable to determine the agent path (cannot make
+                            # the local path relative to the reference path):
+                            # just use the filename
+                            agent_path = local_path.name
                     archive_path = phase_path / agent_path
                     tar.add(local_path, arcname=archive_path)
                     # side effect: strip "local" information

cli/testflinger_cli/tests/test_cli.py

@@ -19,6 +19,8 @@
 """
 
 import json
+import os
+from pathlib import Path
 import re
 import sys
 import tarfile
@@ -107,6 +109,143 @@ def test_submit_bad_data(tmp_path, requests_mock):
     )
 
 
+def test_pack_attachments(tmp_path):
+    """Make sure attachments are packed correctly"""
+
+    attachments = [
+        Path() / "file_0.bin",
+        Path() / "folder" / "file_1.bin",
+    ]
+    attachment_path = tmp_path / "attachments"
+
+    # create attachment files in the attachment path
+    for attachment in attachments:
+        attachment = attachment_path / attachment
+        attachment.parent.mkdir(parents=True)
+        attachment.write_bytes(os.urandom(128))
+
+    attachment_data = {
+        "test": [
+            {
+                # relative local file path
+                "local": str(attachments[0])
+            },
+            {
+                # relative local file path in folder
+                "local": str(attachments[1])
+            },
+            {
+                # absolute local file path, agent path in folder
+                "local": str((attachment_path / attachments[0]).resolve()),
+                "agent": "folder/file_2.bin",
+            },
+            {
+                # relative local path is a directory
+                "local": str(attachments[1].parent),
+                "agent": "folder/deeper/",
+            },
+            {
+                # agent path is absolute (stripped, becomes relative)
+                "local": str(attachments[0]),
+                "agent": "/file_3.bin",
+            },
+        ]
+    }
+
+    # the job.yaml is also in the attachments path
+    # (and relative attachment paths are interpreted in relation to that)
+    sys.argv = ["", "submit", f"{attachment_path}/job.yaml"]
+    tfcli = testflinger_cli.TestflingerCli()
+    archive = tmp_path / "attachments.tar.gz"
+    tfcli.pack_attachments(archive, attachment_data)
+
+    with tarfile.open(archive) as archive:
+        filenames = archive.getnames()
+        print(filenames)
+        assert "test/file_0.bin" in filenames
+        assert "test/folder/file_1.bin" in filenames
+        assert "test/folder/file_2.bin" in filenames
+        assert "test/folder/deeper/file_1.bin" in filenames
+        assert "test/file_3.bin" in filenames
+
+
+def test_pack_attachments_with_reference(tmp_path):
+    """Make sure attachments are packed correctly when using a reference"""
+
+    attachments = [
+        Path() / "file_0.bin",
+        Path() / "folder" / "file_1.bin",
+    ]
+    attachment_path = tmp_path / "attachments"
+
+    # create attachment files
+    for attachment in attachments:
+        attachment = attachment_path / attachment
+        attachment.parent.mkdir(parents=True)
+        attachment.write_bytes(os.urandom(128))
+
+    attachment_data = {
+        "test": [
+            {
+                # relative local file path
+                "local": str(attachments[0])
+            },
+            {
+                # relative local file path in folder
+                "local": str(attachments[1])
+            },
+            {
+                # absolute local file path, agent path in folder
+                "local": str((attachment_path / attachments[0]).resolve()),
+                "agent": "folder/file_2.bin",
+            },
+            {
+                # relative local path is a directory
+                "local": str(attachments[1].parent),
+                "agent": "folder/deeper/",
+            },
+            {
+                # agent path is absolute (stripped, becomes relative)
+                "local": str(attachments[0]),
+                "agent": "/file_3.bin",
+            },
+        ]
+    }
+
+    # the job.yaml is in the tmp_path
+    # (so packing the attachments with fail without specifying that
+    # attachments are relative to the attachments path)
+    sys.argv = ["", "submit", f"{tmp_path}/job.yaml"]
+    tfcli = testflinger_cli.TestflingerCli()
+    archive = tmp_path / "attachments.tar.gz"
+    with pytest.raises(FileNotFoundError):
+        # this fails because the job file is in `tmp_path` whereas
+        # attachments are in `tmp_path/attachments`
+        tfcli.pack_attachments(archive, attachment_data)
+
+    # the job.yaml is in the tmp_path
+    # (but now attachments are relative to the attachments path)
+    sys.argv = [
+        "",
+        "submit",
+        f"{tmp_path}/job.yaml",
+        "--attachments-relative-to",
+        f"{attachment_path}",
+    ]
+    tfcli = testflinger_cli.TestflingerCli()
+    archive = tmp_path / "attachments.tar.gz"
+    tfcli.pack_attachments(archive, attachment_data)
+
+    with tarfile.open(archive) as archive:
+        filenames = archive.getnames()
+        print(filenames)
+        assert "test/file_0.bin" in filenames
+        assert "test/folder/file_1.bin" in filenames
+        assert "test/folder/file_2.bin" in filenames
+        assert "test/folder/deeper/file_1.bin" in filenames
+        assert "test/file_3.bin" in filenames
+
+
 def test_submit_with_attachments(tmp_path):
     """Make sure jobs with attachments are submitted correctly"""
 
@@ -157,7 +296,7 @@ def test_submit_with_attachments(tmp_path):
         with tarfile.open("attachments.tar.gz") as attachments:
             filenames = attachments.getnames()
             assert len(filenames) == 1
-            attachments.extract(filenames[0], filter="data")
+            attachments.extract(filenames[0])
         with open(filenames[0], "r", encoding="utf-8") as attachment:
             assert json.load(attachment) == job_data
 

docs/reference/test-phases.rst

@@ -215,7 +215,9 @@ Example agent configuration:
 
 Attachments
 ------------
-In the `provisioning`, `firmware_update` and `test` phases, it is also possible to specify attachments, i.e. local files that are to be copied over to the Testflinger agent host.
+In the `provisioning`, `firmware_update` and `test` phases, it is also possible
+to specify attachments, i.e. local files that are to be copied over to
+the Testflinger agent host.
 
 * Example job definition:
 
@@ -239,19 +241,28 @@ In the `provisioning`, `firmware_update` and `test` phases, it is also possible
         chmod u+x attachments/test/script.sh
         attachments/test/script.sh
 
-  The `local` fields specify where the attachments are to be found locally, e.g. on the machine where the CLI is executed. For this particular example, this sort of file tree is expected:
+  The `local` fields specify where the attachments are to be found locally,
+  e.g. on the machine where the CLI is executed. Unless otherwise specified,
+  relative paths are interpreted in relation to the location of the Testflinger
+  job file (which is convenient since the job file and the attachments are
+  usually stored together).
+  So for this particular example, this sort of file tree is expected:
 
   .. code-block:: bash
 
     .
+    ├── job.yaml
     ├── config.json
     ├── images
     │   └── ubuntu-logo.png
     ├── scripts
     │   └── my_test_script.sh
     └── ubuntu-22.04.4-preinstalled-desktop-arm64+raspi.img.xz
 
-  On the agent host, the attachments are placed under the `attachments` folder and distributed in separate sub-folders according to phase. If an `agent` field is provided, the attachments are also moved or renamed accordingly. For the example above, the file tree on the agent host would look like this:
+  On the agent host, the attachments are placed under the `attachments` folder
+  and distributed in separate sub-folders according to phase. If an `agent`
+  field is provided, the attachments are also moved or renamed accordingly.
+  For the example above, the file tree on the agent host would look like this:
 
   .. code-block:: bash
 
@@ -267,8 +278,13 @@ In the `provisioning`, `firmware_update` and `test` phases, it is also possible
             │   └── ubuntu-logo.png
             └── script.sh
 
-In this example, there is no `url` field under the `provision_data` to specify where to download the provisioning image from.
-Instead, there is a `use_attachment` field that indicates which attachment should be used as a provisioning image.
+The Testflinger CLI also accepts an optional `--attachments-relative-to` argument.
+When provided, relative paths are interpreted in relation to this reference path,
+instead of the default,  i.e. the location of the Testflinger job file.
+
+In the example above, there is no `url` field under the `provision_data` to specify
+where to download the provisioning image from. Instead, there is a `use_attachment`
+field that indicates which attachment should be used as a provisioning image.
 The presence of *either* `url` or `use_attachment` is required.
 
 At the moment, only the :ref:`muxpi` device connector supports provisioning using an attached image.