From 562097b1ab0c3c984e3fb8f15d2b0f2589d9b3d8 Mon Sep 17 00:00:00 2001 From: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com> Date: Tue, 9 Jul 2024 09:42:20 +0900 Subject: [PATCH] feat: GitHub runner version (#20) * feat: runner version config parsing * feat: runner version pass to cli * chore: remove mac artifacts * chore: ignore mac fs artifacts * chore: revert unneeded charmcraft reformat * fix: charmcraft type * fix: no runner version --- .gitignore | 4 ++ charmcraft.yaml | 38 +++++++------ src/builder.py | 113 +++++++++++++++++++------------------ src/state.py | 36 ++++++++++++ tests/unit/factories.py | 2 + tests/unit/test_builder.py | 92 ++++++++++++++++++++++-------- tests/unit/test_state.py | 69 ++++++++++++++++++++++ 7 files changed, 260 insertions(+), 94 deletions(-) diff --git a/.gitignore b/.gitignore index 1e4d6c7..8de68d7 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,7 @@ __pycache__/ *.egg-info/ */*.rock clouds.yaml + +# Mac filesystem artifact +**/.DS_Store +**./.DS_Store diff --git a/charmcraft.yaml b/charmcraft.yaml index d02e8ff..646b4aa 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -22,25 +22,25 @@ links: type: charm bases: - build-on: - - name: ubuntu - channel: "22.04" - architectures: - - arm64 + - name: ubuntu + channel: "22.04" + architectures: + - arm64 run-on: - - name: ubuntu - channel: "22.04" - architectures: - - arm64 + - name: ubuntu + channel: "22.04" + architectures: + - arm64 - build-on: - - name: ubuntu - channel: "22.04" - architectures: - - amd64 + - name: ubuntu + channel: "22.04" + architectures: + - amd64 run-on: - - name: ubuntu - channel: "22.04" - architectures: - - amd64 + - name: ubuntu + channel: "22.04" + architectures: + - amd64 config: options: @@ -106,6 +106,12 @@ config: type: int default: 5 description: Number of image revisions to keep before deletion. + runner-version: + type: string + default: "" + description: | + The GitHub runner version to use, e.g. 2.317.0. Empty default value will fetch the latest + version by default. See https://github.com/actions/runner/releases. provides: image: diff --git a/src/builder.py b/src/builder.py index bb9361c..9d585f1 100644 --- a/src/builder.py +++ b/src/builder.py @@ -131,28 +131,30 @@ def configure_cron(run_config: state.BuilderRunConfig, interval: int) -> bool: Returns: True if cron is reconfigured. False otherwise. """ - builder_exec_command: str = " ".join( - [ - # HOME path is required for GO modules. - f"HOME={UBUNTU_HOME}", - "/usr/bin/run-one", - "/usr/bin/sudo", - "--preserve-env", - str(GITHUB_RUNNER_IMAGE_BUILDER_PATH), - "run", - run_config.cloud_name, - IMAGE_NAME_TMPL.format( - IMAGE_BASE=run_config.base.value, - ARCH=run_config.arch.value, - ), - "--base-image", - run_config.base.value, - "--keep-revisions", - str(run_config.num_revisions), - "--callback-script", - str(run_config.callback_script.absolute()), - ] - ) + commands = [ + # HOME path is required for GO modules. + f"HOME={UBUNTU_HOME}", + "/usr/bin/run-one", + "/usr/bin/sudo", + "--preserve-env", + str(GITHUB_RUNNER_IMAGE_BUILDER_PATH), + "run", + run_config.cloud_name, + IMAGE_NAME_TMPL.format( + IMAGE_BASE=run_config.base.value, + ARCH=run_config.arch.value, + ), + "--base-image", + run_config.base.value, + "--keep-revisions", + str(run_config.num_revisions), + "--callback-script", + str(run_config.callback_script.absolute()), + ] + if run_config.runner_version: + commands += ["--runner-version", run_config.runner_version] + + builder_exec_command: str = " ".join(commands) cron_text = ( f"0 */{interval} * * * {UBUNTU_USER} {builder_exec_command} " f">> {OUTPUT_LOG_PATH} 2>&1 || {state.FAILED_CALLBACK_SCRIPT_PATH.absolute()}\n" @@ -191,41 +193,44 @@ def run(config: state.BuilderRunConfig) -> None: BuilderRunError: if there was an error while launching the subprocess. """ try: + commands = [ + "(", + # HOME path is required for GO modules. + f"HOME={UBUNTU_HOME}", + "/usr/bin/run-one", + "/usr/bin/sudo", + "--preserve-env", + str(GITHUB_RUNNER_IMAGE_BUILDER_PATH), + "run", + config.cloud_name, + IMAGE_NAME_TMPL.format( + IMAGE_BASE=config.base.value, + ARCH=config.arch.value, + ), + "--base-image", + config.base.value, + "--keep-revisions", + str(config.num_revisions), + "--callback-script", + str(config.callback_script.absolute()), + ] + if config.runner_version: + commands += ["--runner-version", config.runner_version] + commands += [ + ">>", + str(OUTPUT_LOG_PATH), + "2>&1", + "||", + # Run the callback script without Openstack ID argument to let the charm know + # about the error. + str(state.FAILED_CALLBACK_SCRIPT_PATH.absolute()), + ")", + "&", + ] # The callback invokes another hook - which cannot be run when another hook is already # running. Call the process as a background and exit immediately. subprocess.Popen( # pylint: disable=consider-using-with - " ".join( - [ - "(", - # HOME path is required for GO modules. - f"HOME={UBUNTU_HOME}", - "/usr/bin/run-one", - "/usr/bin/sudo", - "--preserve-env", - str(GITHUB_RUNNER_IMAGE_BUILDER_PATH), - "run", - config.cloud_name, - IMAGE_NAME_TMPL.format( - IMAGE_BASE=config.base.value, - ARCH=config.arch.value, - ), - "--base-image", - config.base.value, - "--keep-revisions", - str(config.num_revisions), - "--callback-script", - str(config.callback_script.absolute()), - ">>", - str(OUTPUT_LOG_PATH), - "2>&1", - "||", - # Run the callback script without Openstack ID argument to let the charm know - # about the error. - str(state.FAILED_CALLBACK_SCRIPT_PATH.absolute()), - ")", - "&", - ] - ), + " ".join(commands), # run as shell for log redirection, the command is trusted shell=True, # nosec: B602 user=UBUNTU_USER, diff --git a/src/state.py b/src/state.py index 3b6ec1f..7ec6381 100644 --- a/src/state.py +++ b/src/state.py @@ -31,6 +31,7 @@ OPENSTACK_USER_DOMAIN_CONFIG_NAME = "openstack-user-domain-name" OPENSTACK_USER_CONFIG_NAME = "openstack-user-name" REVISION_HISTORY_LIMIT_CONFIG_NAME = "revision-history-limit" +RUNNER_VERSION_CONFIG_NAME = "runner-version" SUCCESS_CALLBACK_SCRIPT_PATH = pathlib.Path("/home/ubuntu/on_build_success_callback.sh") FAILED_CALLBACK_SCRIPT_PATH = pathlib.Path("/home/ubuntu/on_build_failed_callback.sh") @@ -179,6 +180,7 @@ class BuilderRunConfig: cloud_config: The Openstack clouds.yaml passed as charm config. cloud_name: The Openstack cloud name to connect to from clouds.yaml. num_revisions: Number of images to keep before deletion. + runner_version: The GitHub runner version to embed in the image. Latest version if empty. callback_script: Path to callback script. """ @@ -186,6 +188,7 @@ class BuilderRunConfig: base: BaseImage cloud_config: dict[str, typing.Any] num_revisions: int + runner_version: str callback_script: pathlib.Path = SUCCESS_CALLBACK_SCRIPT_PATH @property @@ -228,6 +231,7 @@ def from_charm(cls, charm: ops.CharmBase) -> "BuilderRunConfig": try: revision_history_limit = _parse_revision_history_limit(charm) + runner_version = _parse_runner_version(charm=charm) except ValueError as exc: raise BuildConfigInvalidError(msg=str(exc)) from exc @@ -236,6 +240,7 @@ def from_charm(cls, charm: ops.CharmBase) -> "BuilderRunConfig": base=base_image, cloud_config=cloud_config, num_revisions=revision_history_limit, + runner_version=runner_version, ) @@ -281,6 +286,37 @@ def _parse_revision_history_limit(charm: ops.CharmBase) -> int: return revision_history +def _parse_runner_version(charm: ops.CharmBase) -> str: + """Parse the runner version configuration value. + + Args: + charm: The charm instance. + + Raises: + ValueError: If an invalid version number is provided. + + Returns: + The semantic version number of the GitHub runner. + """ + version_str = typing.cast(str, charm.config.get(RUNNER_VERSION_CONFIG_NAME, "")) + if not version_str: + return "" + + parts = version_str.split(".") + if len(parts) != 3: + raise ValueError("The runner version must be in semantic version format.") + try: + major = int(parts[0]) + minor = int(parts[1]) + patch = int(parts[2]) + except ValueError as exc: + raise ValueError("The runner version numbers must be an integer.") from exc + if major < 0 or minor < 0 or patch < 0: + raise ValueError("The runner version numbers cannot be negative.") + + return version_str + + class InvalidCloudConfigError(Exception): """Represents an error with openstack cloud config.""" diff --git a/tests/unit/factories.py b/tests/unit/factories.py index 890ab96..6041a46 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -20,6 +20,7 @@ OPENSTACK_USER_CONFIG_NAME, OPENSTACK_USER_DOMAIN_CONFIG_NAME, REVISION_HISTORY_LIMIT_CONFIG_NAME, + RUNNER_VERSION_CONFIG_NAME, ) T = typing.TypeVar("T") @@ -60,6 +61,7 @@ class Meta: # pylint: disable=too-few-public-methods OPENSTACK_USER_DOMAIN_CONFIG_NAME: "user_domain_name", OPENSTACK_USER_CONFIG_NAME: "username", REVISION_HISTORY_LIMIT_CONFIG_NAME: "5", + RUNNER_VERSION_CONFIG_NAME: "1.234.5", } ) diff --git a/tests/unit/test_builder.py b/tests/unit/test_builder.py index 54e545c..9aee505 100644 --- a/tests/unit/test_builder.py +++ b/tests/unit/test_builder.py @@ -184,6 +184,7 @@ def test_configure_cron_no_reconfigure(monkeypatch: pytest.MonkeyPatch): base=state.BaseImage.JAMMY, cloud_config=factories.CloudFactory(), callback_script=MagicMock(), + runner_version="1.234.5", num_revisions=5, ), interval=1, @@ -192,7 +193,60 @@ def test_configure_cron_no_reconfigure(monkeypatch: pytest.MonkeyPatch): service_restart_mock.assert_not_called() -def test_configure_cron(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): +@pytest.mark.parametrize( + "run_config, expected_file_contents", + [ + pytest.param( + state.BuilderRunConfig( + arch=state.Arch.ARM64, + base=state.BaseImage.JAMMY, + cloud_config=factories.CloudFactory(), + runner_version="1.234.5", + num_revisions=5, + ), + """0 */1 * * * ubuntu HOME=/home/ubuntu /usr/bin/run-one \ +/usr/bin/sudo \ +--preserve-env /home/ubuntu/.local/bin/github-runner-image-builder run \ +testcloud \ +jammy-arm64 \ +--base-image jammy \ +--keep-revisions 5 \ +--callback-script {TEST_PATH} \ +--runner-version 1.234.5 \ +>> /home/ubuntu/github-runner-image-builder.log 2>&1 \ +|| /home/ubuntu/on_build_failed_callback.sh +""", + id="runner version set", + ), + pytest.param( + state.BuilderRunConfig( + arch=state.Arch.ARM64, + base=state.BaseImage.JAMMY, + cloud_config=factories.CloudFactory(), + runner_version="", + num_revisions=5, + ), + """0 */1 * * * ubuntu HOME=/home/ubuntu /usr/bin/run-one \ +/usr/bin/sudo \ +--preserve-env /home/ubuntu/.local/bin/github-runner-image-builder run \ +testcloud \ +jammy-arm64 \ +--base-image jammy \ +--keep-revisions 5 \ +--callback-script {TEST_PATH} \ +>> /home/ubuntu/github-runner-image-builder.log 2>&1 \ +|| /home/ubuntu/on_build_failed_callback.sh +""", + id="runner version not set", + ), + ], +) +def test_configure_cron( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + run_config: state.BuilderRunConfig, + expected_file_contents: str, +): """ arrange: given monkeypatched subfunctions of configure_cron. act: when configure_cron is called. @@ -203,32 +257,13 @@ def test_configure_cron(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): test_path = tmp_path / "test" monkeypatch.setattr(builder, "CRON_BUILD_SCHEDULE_PATH", test_path) test_interval = 1 - test_config = state.BuilderRunConfig( - arch=state.Arch.ARM64, - base=state.BaseImage.JAMMY, - cloud_config=factories.CloudFactory(), - callback_script=test_path, - num_revisions=5, - ) + run_config.callback_script = test_path - builder.configure_cron(run_config=test_config, interval=test_interval) + builder.configure_cron(run_config=run_config, interval=test_interval) service_restart_mock.assert_called_once() cron_contents = test_path.read_text(encoding="utf-8") - assert ( - cron_contents - == f"""0 */{test_interval} * * * ubuntu HOME=/home/ubuntu /usr/bin/run-one \ -/usr/bin/sudo \ ---preserve-env /home/ubuntu/.local/bin/github-runner-image-builder run \ -{test_config.cloud_name} \ -{test_config.base.value}-{test_config.arch.value} \ ---base-image {test_config.base.value} \ ---keep-revisions {test_config.num_revisions} \ ---callback-script {test_path} \ ->> /home/ubuntu/github-runner-image-builder.log 2>&1 \ -|| /home/ubuntu/on_build_failed_callback.sh -""" - ) + assert cron_contents == expected_file_contents.format(TEST_PATH=test_path) def test__should_configure_cron_no_path(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): @@ -272,6 +307,7 @@ def test_run_error(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): base=state.BaseImage.JAMMY, cloud_config=factories.CloudFactory(), num_revisions=1, + runner_version="1.234.5", callback_script=tmp_path, ) @@ -281,7 +317,14 @@ def test_run_error(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): assert "Failed to spawn process" in str(exc.getrepr()) -def test_run(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): +@pytest.mark.parametrize( + "runner_version", + [ + pytest.param("1.234.5", id="runner version config set"), + pytest.param("", id="runner version config not set"), + ], +) +def test_run(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, runner_version: str): """ arrange: given a monkeypatched subprocess call. act: when run is called. @@ -293,6 +336,7 @@ def test_run(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): base=state.BaseImage.JAMMY, cloud_config=factories.CloudFactory(), num_revisions=1, + runner_version=runner_version, callback_script=tmp_path, ) diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index 76e25db..33464da 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -179,6 +179,13 @@ def test_proxy_config(monkeypatch: pytest.MonkeyPatch): "", id="invalid revision history", ), + pytest.param( + state, + "_parse_runner_version", + ValueError, + "", + id="invalid runner version", + ), ], ) def test_builder_run_config_invalid_configs( @@ -197,6 +204,7 @@ def test_builder_run_config_invalid_configs( monkeypatch.setattr(state.BaseImage, "from_charm", MagicMock()) monkeypatch.setattr(state, "_parse_openstack_clouds_config", MagicMock()) monkeypatch.setattr(state, "_parse_revision_history_limit", MagicMock()) + monkeypatch.setattr(state, "_parse_runner_version", MagicMock()) monkeypatch.setattr(module, patch_func, MagicMock(side_effect=exception)) charm = MockCharmFactory() @@ -236,6 +244,7 @@ def test_builder_run_config(monkeypatch: pytest.MonkeyPatch): } } }, + runner_version="1.234.5", num_revisions=5, ), interval=6, @@ -349,6 +358,66 @@ def test__parse_revision_history_limit(revision_history: str, expected: int): assert state._parse_revision_history_limit(charm) == expected +@pytest.mark.parametrize( + "version, expected_message", + [ + pytest.param( + "abc", "The runner version must be in semantic version format.", id="not a version" + ), + pytest.param( + "1.20", + "The runner version must be in semantic version format.", + id="not a semantic version", + ), + pytest.param( + "a.b.c", + "The runner version numbers must be an integer.", + id="non a integer versions", + ), + pytest.param( + "1.20.-1", + "The runner version numbers cannot be negative", + id="not a semantic version (negative integer)", + ), + pytest.param( + "v1.20.1", "The runner version numbers must be an integer", id="v char not accepted" + ), + ], +) +def test__parse_runner_version_invalid(version: str, expected_message: str): + """ + arrange: given an invalid runner version config value. + act: when _parse_runner_version is called. + assert: ValueError is raised. + """ + charm = MockCharmFactory() + charm.config[state.RUNNER_VERSION_CONFIG_NAME] = version + + with pytest.raises(ValueError) as exc: + state._parse_runner_version(charm) + + assert expected_message in str(exc.getrepr()) + + +@pytest.mark.parametrize( + "version, expected_version", + [ + pytest.param("", "", id="latest version"), + pytest.param("1.234.5", "1.234.5", id="valid version"), + ], +) +def test__parse_runner_version(version: str, expected_version: str): + """ + arrange: given a valid runner version config value. + act: when _parse_runner_version is called. + assert: expected version number is returned. + """ + charm = MockCharmFactory() + charm.config[state.RUNNER_VERSION_CONFIG_NAME] = version + + assert state._parse_runner_version(charm) == expected_version + + @pytest.mark.parametrize( "missing_config", [