feat: enables backup retention and collation setting for SQL DB#189
feat: enables backup retention and collation setting for SQL DB#189dzsquared wants to merge 3 commits intomicrosoft:mainfrom
Conversation
|
hi @aviatco - can this be reviewed for the next release? |
|
|
||
| @staticmethod | ||
| def invalid_parameter_format(invalidParam: str, expectedFormat: str) -> str: | ||
| return f"Invalid parameter format: '{invalidParam}'. Expected format: {expectedFormat}" |
There was a problem hiding this comment.
| return f"Invalid parameter format: '{invalidParam}'. Expected format: {expectedFormat}" | |
| return f"Invalid parameter data type: '{invalidParam}'. Expected data type: {expected_data_type}" |
| return "A folder with the same name already exists" | ||
|
|
||
| @staticmethod | ||
| def invalid_parameter_format(invalidParam: str, expectedFormat: str) -> str: |
There was a problem hiding this comment.
In Python, snake_case is the convention of writing names in all lowercase with words separated by underscores
| def invalid_parameter_format(invalidParam: str, expectedFormat: str) -> str: | |
| def invalid_parameter_format(invalid_param: str, expected_data_type: str) -> str: |
| _collation = params.get("collation") | ||
|
|
||
| if _mode or _backup_retention_days or _collation: | ||
| creation_payload: dict = {"creationMode": _mode if _mode else "new"} |
There was a problem hiding this comment.
why we assume if creatIonmode not specified it should be 'new'? isn't it a required parameter for creationPayload?
| ] | ||
| } | ||
|
|
||
| case ItemType.SQL_DATABASE: |
There was a problem hiding this comment.
what about restore creation payload?
| case ItemType.MOUNTED_DATA_FACTORY: | ||
| required_params = ["subscriptionId", "resourceGroup", "factoryName"] | ||
| case ItemType.SQL_DATABASE: | ||
| optional_params = ["mode", "backupRetentionDays", "collation"] |
|
|
||
| if _mode or _backup_retention_days or _collation: | ||
| creation_payload: dict = {"creationMode": _mode if _mode else "new"} | ||
| if _backup_retention_days: |
There was a problem hiding this comment.
will not run the conversion if the value is "0" or 0, because both are falsy. That means:
• "0" → skipped entirely
• 0 → skipped entirely
use
if _backup_retention_days is not None:
try:
creation_payload["backupRetentionDays"] = int(_backup_retention_days)
except (ValueError, TypeError):
raise FabricCLIError(
ErrorMessages.Mkdir.invalid_parameter_format(
_backup_retention_days, "integer"
),
fab_constant.ERROR_INVALID_INPUT,
)
| rm(item_full_path) | ||
|
|
||
| # region SQL database | ||
| def test_mkdir_sqldatabase_without_params_success( |
There was a problem hiding this comment.
redundant - already covered in test_mkdir_item_success. please remove the recording file as well
| # Cleanup | ||
| rm(sqldb_full_path) | ||
|
|
||
| def test_mkdir_sqldatabase_partial_params_success( |
There was a problem hiding this comment.
Please define another case in mkdir_item_with_creation_payload_success_params in conftest instead of adding new test.
| # Assert failure due to invalid integer | ||
| assert_fabric_cli_error(constant.ERROR_INVALID_INPUT) | ||
|
|
||
| def test_mkdir_sqldatabase_explicit_mode_success( |
There was a problem hiding this comment.
same as comment above - define this usecase in the conftest parameterize and remove this test and its recording
| # Cleanup | ||
| rm(sqldb_full_path) | ||
|
|
||
| def test_mkdir_sqldatabase_param_discovery( |
There was a problem hiding this comment.
should not be as e2e test, we should use the test_mkdir_utils you added and add the test there and remove the recording.
2 participants
📥 Pull Request
✨ Description of new changes
fixes #148
edit: fantastic, I broke the tests while fighting with the formatter to avoid a huge changeset