[SVRENG-609] boardwalk (bug): correct job class initialization (#86)

* [SVRENG-609] boardwalk (bug): correct job class initialization

Boardwalk release 0.8.22 introduced a bug where options supplied to Jobs would cause a TypeError, due to the options parameter not being included in the declaration of the init method.

This has been fixed, and to guard against similar mishaps, a test suite to exercise these regressions was added into the overall test suite, with focus on the options, as well as testing preconditions.

Makefile

@@ -84,10 +84,14 @@ render-d2:
 .PHONY: test
 test: test-pytest test-ruff test-pyright test-semgrep
 
-# Run pytest
+# Run pytest verbosely if we're running manually, but normally if we're in a CI environment.
 .PHONY: test-pytest
 test-pytest: develop
+ifndef CI
+	poetry run pytest  --verbose
+else
 	poetry run pytest
+endif
 
 # Run all available Ruff checks
 .PHONY: test-ruff

pyproject.toml

@@ -8,7 +8,7 @@ requires-python = ">=3.11"
 
 [tool.poetry]
 name = "boardwalk"
-version = "0.8.22"
+version = "0.8.23"
 description = "Boardwalk is a linear Ansible workflow engine"
 readme = "README.md"
 authors = [

src/boardwalk/ansible.py

@@ -47,7 +47,7 @@ class RunnerKwargs(TypedDict, total=False):
         suppress_env_files: bool
         playbook: RunnerPlaybook | AnsibleTasksType
         verbosity: int
-        extravars: dict | None
+        extravars: dict[str, Any] | None
 
     class RunnerKwargsEnvvars(TypedDict, total=False):
         ANSIBLE_BECOME_ASK_PASS: str
@@ -120,6 +120,7 @@ def ansible_runner_run_tasks(
     quiet: bool = True,
     timeout: int | None = None,
     verbosity: int = 0,
+    extra_vars: dict = {},
 ) -> ansible_runner.Runner:
     """
     Wraps ansible_runner.run to run Ansible tasks with some defaults for
@@ -161,7 +162,7 @@ def ansible_runner_run_tasks(
     if job_type == boardwalk.manifest.JobTypes.PLAYBOOK:
         # Executing a (list of) playbook(s) requires some different settings
         runner_kwargs["limit"] = hosts
-        runner_kwargs["extravars"] = {"boardwalk_operation": True}
+        runner_kwargs["extravars"] = {"boardwalk_operation": True} | extra_vars
         runner_kwargs["playbook"] = tasks
 
     output_msg_prefix = f"{hosts}: ansible_runner invocation"

src/boardwalk/cli_run.py

@@ -697,6 +697,7 @@ def execute_workflow_jobs(host: Host, workspace: Workspace, job_kind: str, verbo
                 invocation_msg=f"{job_kind}_{job.job_type.name}_Job_{job.name}",
                 quiet=False,
                 tasks=tasks,
+                extra_vars=job.options,
             )
 
 
@@ -754,6 +755,7 @@ def execute_host_workflow(host: Host, workspace: Workspace, verbosity: int):
         )
     host.set_remote_state(remote_state, become_password, _check_mode)
 
+    logger.success(f"{host.name}: Host completed successfully; wrapping up")
     if boardwalkd_client:
         boardwalkd_client.queue_event(
             WorkspaceEvent(

src/boardwalk/host.py

@@ -49,6 +49,7 @@ def ansible_run(
         check: bool = False,
         gather_facts: bool = True,
         quiet: bool = True,
+        extra_vars: dict = {},
     ) -> Runner:
         """
         Wraps ansible_runner_run_tasks for performing Ansible tasks against this host
@@ -64,6 +65,7 @@ def ansible_run(
             check=check,
             gather_facts=gather_facts,
             quiet=quiet,
+            extra_vars=extra_vars,
         )
 
     def is_locked(self) -> str | bool:

src/boardwalk/manifest.py

@@ -154,8 +154,8 @@ def _check_options(self, options: dict[str, Any]):
 class TaskJob(BaseJob):
     """Defines a single Job as methods, used to execute Tasks"""
 
-    def __init__(self):
-        super().__init__()
+    def __init__(self, options: dict[str, Any] = dict()):
+        super().__init__(options=options)
         self.job_type = JobTypes.TASK
 
     def tasks(self) -> AnsibleTasksType:
@@ -171,20 +171,21 @@ class Job(TaskJob):
     Deprecated in favor of TaskJob.
     """
 
-    def __init__(self):
+    def __init__(self, options: dict[str, Any] = dict()):
         warnings.warn(
             "The job type Job is deprecated, and will be removed in a future release. Use TaskJob or PlaybookJob, as appropriate.",
             DeprecationWarning,
         )
-        super().__init__()
+        super().__init__(options=options)
 
 
 class PlaybookJob(BaseJob):
     """Defines a single Job as methods, used to execute Playbooks"""
 
-    def __init__(self):
-        super().__init__()
+    def __init__(self, options: dict[str, Any] = dict()):
+        super().__init__(options=options)
         self.job_type = JobTypes.PLAYBOOK
+        self.extra_vars = options
 
     def tasks(self) -> AnsibleTasksType:
         """Helper method used to return the contents of the self.playbooks() function."""

test/boardwalk/test_manifest.py

@@ -1,6 +1,39 @@
 import pytest
 
-from boardwalk import Job
+from boardwalk import Job, PlaybookJob, TaskJob
+from boardwalk.manifest import JobTypes
+
+
+@pytest.mark.parametrize(
+    ("job_class", "job_type"),
+    [
+        pytest.param(TaskJob, JobTypes.TASK),
+        pytest.param(
+            Job,
+            JobTypes.TASK,
+            marks=pytest.mark.filterwarnings("ignore:The job type Job is deprecated:DeprecationWarning"),
+        ),
+        pytest.param(PlaybookJob, JobTypes.PLAYBOOK),
+    ],
+)
+def test_verify_job_types_match_expected_types(job_class, job_type):
+    job = job_class()
+    assert job_type == job.job_type
+
+
+@pytest.mark.parametrize(
+    ("job_class", "function_name"),
+    [
+        pytest.param(TaskJob, "tasks"),
+        pytest.param(Job, "tasks"),
+        pytest.param(PlaybookJob, "playbooks"),
+        pytest.param(PlaybookJob, "tasks"),
+    ],
+)
+def test_verify_job_classes_have_expected_task_functions(job_class, function_name):
+    # The Job type needs to have the expected function name, and also be a callable function
+    assert hasattr(job_class, function_name)
+    assert callable(eval(f"{job_class.__name__}.{function_name}"))
 
 
 def test_using_not_differentiated_Job_class_warns_about_deprecation():

test/integration/test_workspaces.py

@@ -1,4 +1,5 @@
 import os
+import platform
 import signal
 from pathlib import Path
 from typing import Any
@@ -16,6 +17,28 @@
         pytest.param("ShouldSucceedTestWorkspace", False, ""),
         pytest.param("ShouldSucceedPlaybookExecutionTestWorkspace", False, ""),
         pytest.param("UITestVeryVeryLongWorkSpaceNameWorkspace", False, ""),
+        # Next four are from test/server-client/pylib/regression_bz_svreng_608.py
+        pytest.param("TaskJobWithOptionsShouldSucceedWorkspace", False, ""),
+        pytest.param("PlaybookJobWithOptionsShouldSucceedWorkspace", False, ""),
+        pytest.param(
+            "TaskJobWithPreconditionsShouldSucceedIfHostIsMacOSXWorkspace",
+            False,
+            "",
+            marks=pytest.mark.skipif(
+                condition="macos" not in platform.platform().lower(),
+                reason="Workspace's preconditions depends on the host being MacOS.",
+            ),
+        ),
+        # Technically this one doesn't _fail_, but this lets it fit neatly into the parameterized tests.
+        pytest.param(
+            "TaskJobWithPreconditionsShouldBeSkippedIfHostIsMacOSXWorkspace",
+            True,
+            "No hosts meet preconditions",
+            marks=pytest.mark.skipif(
+                condition="macos" not in platform.platform().lower(),
+                reason="Workspace's preconditions depends on the host being MacOS.",
+            ),
+        ),
         pytest.param(
             "ShouldFailTestWorkspace",
             True,
@@ -54,15 +77,19 @@ async def test_development_workspaces(
             with fail_after(delay=90) as scope:
                 async with await open_process(command=command, env=new_environ) as process:
                     async for text in TextReceiveStream(process.stdout):  # type:ignore
+                        # To allow for reading what was received, if the test ends up failing.
+                        print(text)
                         output_stdout.append(text)
                         if failure_expected and "Waiting for release before continuing" in text:
                             process.send_signal(signal.SIGINT)
                     async for text in TextReceiveStream(process.stderr):  # type:ignore
+                        print(text)
                         output_stderr.append(text)
 
         assert not scope.cancelled_caught
 
     if failure_expected:
         assert failure_msg in "".join(output_stdout)
     else:
+        assert "Host completed successfully; wrapping up" in "".join(output_stdout)
         assert process.returncode == 0

test/server-client/Boardwalkfile.py

@@ -3,6 +3,8 @@
 import os
 from typing import TYPE_CHECKING
 
+from pylib.regression_bz_svreng_609 import *  # noqa: F403
+
 from boardwalk import PlaybookJob, TaskJob, Workflow, Workspace, WorkspaceConfig, path
 
 if TYPE_CHECKING:
@@ -147,8 +149,8 @@ class ShouldSucceedPlaybookExecutionTestJob(PlaybookJob):
 
     def playbooks(self) -> AnsibleTasksType:
         return [
-            {"ansible.builtin.import_playbook": path("playbook-job-test-should-succeed.yml")},
-            {"ansible.builtin.import_playbook": path("playbook-job-test-should-be-skipped.yml")},
+            {"ansible.builtin.import_playbook": path("playbooks/playbook-job-test-should-succeed.yml")},
+            {"ansible.builtin.import_playbook": path("playbooks/playbook-job-test-should-be-skipped.yml")},
         ]
 
 
@@ -159,7 +161,7 @@ class ShouldFailPlaybookExecutionTestJob(PlaybookJob):
 
     def playbooks(self) -> AnsibleTasksType:
         return [
-            {"ansible.builtin.import_playbook": path("playbook-job-test-should-succeed.yml")},
-            {"ansible.builtin.import_playbook": path("playbook-job-test-should-be-skipped.yml")},
-            {"ansible.builtin.import_playbook": path("playbook-job-test-should-fail.yml")},
+            {"ansible.builtin.import_playbook": path("playbooks/playbook-job-test-should-succeed.yml")},
+            {"ansible.builtin.import_playbook": path("playbooks/playbook-job-test-should-be-skipped.yml")},
+            {"ansible.builtin.import_playbook": path("playbooks/playbook-job-test-should-fail.yml")},
         ]

test/server-client/playbooks/playbook-job-test-echo-variable.yml

@@ -0,0 +1,7 @@
+---
+- name: Test echoing a set variable
+  hosts: localhost
+  tasks:
+    - name: Tell me my variable, please!
+      ansible.builtin.debug:
+        msg: "Your variable is: {{ boardwalk_playbookjob_test_variable }}"