Skip to content

Commit

Permalink
Allow non-string infra defaults for plugins (#1921)
Browse files Browse the repository at this point in the history
* method to handle various py-to-tf conversions

* update changelog

* make staticmethod

* add convert to tfvar test

* fix changelog typo

* remove getattr patch; assert new method called

* fake infra defaults to test conversion in `up`
  • Loading branch information
araghukas authored Feb 1, 2024
1 parent 8d22e7d commit 8598589
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 8 deletions.
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

0 comments on commit 8598589

Please sign in to comment.