Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-string infra defaults for plugins #1921

Merged
merged 7 commits into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- Added CRM method to handle Python to TF value conversion (e.g. None->null, True->true, False->false).
- Added `pennylane` as a requirement in tests due to the tutorials using it

### Changed
Expand Down
32 changes: 29 additions & 3 deletions covalent/cloud_resource_manager/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from configparser import ConfigParser
from pathlib import Path
from types import ModuleType
from typing import Callable, Dict, List, Optional, Union
from typing import Any, Callable, Dict, List, Optional, Sequence, Union

from covalent._shared_files.config import set_config
from covalent.executor import _executor_manager
Expand Down Expand Up @@ -432,8 +432,8 @@ def up(self, print_callback: Callable, dry_run: bool = True) -> None:
if "default" in value:
tf_vars_env_dict[f"TF_VAR_{key}"] = value["default"]

if value["default"] != "":
f.write(f'{key}="{value["default"]}"\n')
if value["default"]:
f.write(f'{key}={self._convert_to_tfvar(value["default"])}\n')

# Overwrite the default values with the user passed values
if self.executor_options:
Expand Down Expand Up @@ -537,3 +537,29 @@ def status(self) -> None:

# Run `terraform state list`
return self._run_in_subprocess(cmd=tf_state, env_vars=self._terraform_log_env_vars)

@staticmethod
def _convert_to_tfvar(value: Any) -> Any:
"""
Convert the value to a string that can be parsed as a terraform variable.

Args:
value: Value to convert

Returns:
Converted value

"""
if value is True:
return "true"
if value is False:
return "false"
if value is None:
return "null"
if isinstance(value, str):
return f'"{value}"'
if isinstance(value, Sequence):
values = [CloudResourceManager._convert_to_tfvar(v) for v in value]
return f"[{', '.join(values)}]"

return str(value)
44 changes: 39 additions & 5 deletions tests/covalent_tests/cloud_resource_manager/core_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ def executor_module_path():
return "test_executor_module_path"


@pytest.fixture
def executor_infra_defaults():
from pydantic import BaseModel

class FakeExecutorInfraDefaults(BaseModel):
string_param: str = "fake_address_123"
number_param: int = 123
sequence_param: tuple = (1, 2, 3)

return FakeExecutorInfraDefaults


@pytest.fixture
def crm(mocker, executor_name, executor_module_path):
mocker.patch(
Expand Down Expand Up @@ -377,7 +389,9 @@ def test_get_tf_statefile_path(mocker, crm, executor_name):
(False, {"test_key": "test_value"}),
],
)
def test_up(mocker, dry_run, executor_options, executor_name, executor_module_path):
def test_up(
mocker, dry_run, executor_options, executor_name, executor_module_path, executor_infra_defaults
):
"""
Unit test for CloudResourceManager.up() method
"""
Expand All @@ -401,10 +415,6 @@ def test_up(mocker, dry_run, executor_options, executor_name, executor_module_pa
"covalent.cloud_resource_manager.core.get_executor_module",
)

mocker.patch(
"covalent.cloud_resource_manager.core.getattr",
)

# Mocking as we are instantiating with executor options
mocker.patch(
"covalent.cloud_resource_manager.core.validate_options",
Expand Down Expand Up @@ -438,6 +448,9 @@ def test_up(mocker, dry_run, executor_options, executor_name, executor_module_pa
options=executor_options,
)

# Override infra defaults with dummy values.
crm.ExecutorInfraDefaults = executor_infra_defaults

with mock.patch(
"covalent.cloud_resource_manager.core.open",
mock.mock_open(),
Expand Down Expand Up @@ -652,6 +665,27 @@ def test_crm_get_resource_status(mocker, crm):
mock_terraform_error_validator.assert_called_once()


def test_crm_convert_to_tfvar(mocker, crm):
"""
Unit test for CloudResourceManager._convert_to_tfvar() method.

Test conversion outcomes.
"""
_values_map = {
# Convenient test case (not valid Terraform):
(1, False, None, "covalent321"): '[1, false, null, "covalent321"]',
# Usual test cases:
True: "true",
False: "false",
None: "null",
"covalent123": '"covalent123"', # must include quotes
16: "16",
}

for _value, _expected in _values_map.items():
assert crm._convert_to_tfvar(_value) == _expected


def test_no_git(crm, mocker):
"""
Test for exit with status 1 if `git` is not available.
Expand Down
Loading