Skip to content

Commit

Permalink
🐙 octavia-cli: make update logic clearer (#11386)
Browse files Browse the repository at this point in the history
  • Loading branch information
@alafanechere
alafanechere authored Mar 25, 2022
1 parent 966849c commit 3346edc
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 98 deletions.
27 changes: 16 additions & 11 deletions octavia-cli/octavia_cli/apply/commands.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#

from glob import glob
from typing import List, Tuple
from typing import List, Optional, Tuple

import airbyte_api_client
import click
Expand Down Expand Up @@ -63,25 +63,27 @@ def apply_single_resource(resource: BaseResource, force: bool) -> None:
click.echo("\n".join(messages))


def should_update_resource(force: bool, diff: str, local_file_changed: bool) -> Tuple[bool, str]:
def should_update_resource(force: bool, user_validation: Optional[bool], local_file_changed: bool) -> Tuple[bool, str]:
"""Function to decide if the resource needs an update or not.
Args:
force (bool): Whether force mode is on.
diff (str): The computed diff between local and remote for this resource.
user_validation (bool): User validated the existing changes.
local_file_changed (bool): Whether the local file describing the resource was modified.
Returns:
Tuple[bool, str]: Boolean to know if resource should be updated and string describing the update reason.
"""
if force:
should_update, update_reason = True, "🚨 - Running update because force mode is on."
elif diff:
should_update, update_reason = True, "✍️ - Running update because a diff was detected between local and remote resource."
elif local_file_changed:
should_update, update_reason = True, "🚨 - Running update because the force mode is activated."
elif user_validation is True:
should_update, update_reason = True, "🟢 - Running update because you validated the changes."
elif user_validation is False:
should_update, update_reason = False, "🔴 - Did not update because you refused the changes."
elif user_validation is None and local_file_changed:
should_update, update_reason = (
True,
"✍️ - Running update because a local file change was detected and a secret field might have been edited.",
"🟡 - Running update because a local file change was detected and a secret field might have been edited.",
)
else:
should_update, update_reason = False, "😴 - Did not update because no change detected."
Expand Down Expand Up @@ -135,11 +137,14 @@ def update_resource(resource: BaseResource, force: bool) -> List[str]:
Returns:
List[str]: Post update messages to display to standard output.
"""
output_messages = []
diff = resource.get_diff_with_remote_resource()
should_update, update_reason = should_update_resource(force, diff, resource.local_file_changed)
output_messages = [update_reason]
user_validation = None
if not force and diff:
should_update = prompt_for_diff_validation(resource.resource_name, diff)
user_validation = prompt_for_diff_validation(resource.resource_name, diff)
should_update, update_reason = should_update_resource(force, user_validation, resource.local_file_changed)
click.echo(update_reason)

if should_update:
updated_resource, state = resource.update()
output_messages.append(
Expand Down
173 changes: 86 additions & 87 deletions octavia-cli/unit_tests/test_apply/test_commands.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,47 +94,74 @@ def test_apply_single_resource(patch_click, mocker, resource_was_created):


@pytest.mark.parametrize(
"diff,local_file_changed,force,expected_should_update,expected_update_reason",
"force,user_validation,local_file_changed,expect_update,expected_reason",
[
(True, True, True, True, "🚨 - Running update because force mode is on."), # check if force has the top priority
(True, False, True, True, "🚨 - Running update because force mode is on."), # check if force has the top priority
(True, False, False, True, "🚨 - Running update because force mode is on."), # check if force has the top priority
(True, True, False, True, "🚨 - Running update because force mode is on."), # check if force has the top priority
(
pytest.param(
True, True, True, True, "🚨 - Running update because the force mode is activated.", id="1 - Check if force has the top priority."
),
pytest.param(
True,
False,
True,
True,
"🚨 - Running update because the force mode is activated.",
id="2 - Check if force has the top priority.",
),
pytest.param(
True,
"✍️ - Running update because a diff was detected between local and remote resource.",
), # check if diff has priority of file changed
(
False,
True,
False,
True,
"✍️ - Running update because a diff was detected between local and remote resource.",
), # check if diff has priority of file changed
(
"🚨 - Running update because the force mode is activated.",
id="3 - Check if force has the top priority.",
),
pytest.param(
True,
True,
False,
True,
"🚨 - Running update because the force mode is activated.",
id="4 - Check if force has the top priority.",
),
pytest.param(
False,
True,
True,
"✍️ - Running update because a local file change was detected and a secret field might have been edited.",
), # check if local_file_changed runs even if no diff found
(
True,
"🟢 - Running update because you validated the changes.",
id="Check if user validation has priority over local file change.",
),
pytest.param(
False,
False,
True,
False,
"🔴 - Did not update because you refused the changes.",
id="Check if user validation has priority over local file change.",
),
pytest.param(
False,
None,
True,
True,
"🟡 - Running update because a local file change was detected and a secret field might have been edited.",
id="Check if local_file_changed runs even if user validation is None.",
),
pytest.param(
False,
None,
False,
False,
"😴 - Did not update because no change detected.",
), # check if local_file_changed runs even if no diff found
id="Check no update if no local change and user validation is None.",
),
],
)
def test_should_update_resource(patch_click, mocker, diff, local_file_changed, force, expected_should_update, expected_update_reason):
should_update, update_reason = commands.should_update_resource(diff, local_file_changed, force)
assert should_update == expected_should_update
def test_should_update_resource(patch_click, mocker, force, user_validation, local_file_changed, expect_update, expected_reason):
should_update, update_reason = commands.should_update_resource(force, user_validation, local_file_changed)
assert should_update == expect_update
assert update_reason == commands.click.style.return_value
commands.click.style.assert_called_with(expected_update_reason, fg="green")
commands.click.style.assert_called_with(expected_reason, fg="green")


@pytest.mark.parametrize(
Expand Down Expand Up @@ -177,80 +204,52 @@ def test_create_resource(patch_click, mocker):


@pytest.mark.parametrize(
"force,diff,should_update_resource,expect_prompt,user_validate_diff,expect_update,expected_number_of_messages",
"force,diff,local_file_changed,expect_prompt,user_validation,expect_update",
[
(True, True, True, False, False, True, 3), # Force is on, we have a diff, prompt should not be displayed: we expect update.
(
True,
False,
True,
False,
False,
True,
3,
), # Force is on, no diff, should_update_resource == true, prompt should not be displayed, we expect update.
(
True,
False,
False,
False,
False,
False,
1,
), # Force is on, no diff, should_update_resource == false, prompt should not be displayed, we don't expect update. This scenario should not exists in current implementation as force always trigger update.
(
False,
True,
True,
True,
True,
True,
3,
), # Force is off, we have diff, prompt should be displayed, user validate diff: we expected update.
(
False,
False,
True,
False,
False,
True,
3,
), # Force is off, no diff, should_update_resource == true (in case of file change), prompt should not be displayed, we expect update.
(
False,
True,
True,
True,
False,
False,
1,
), # Force is off, we have a diff but the user does not validate it: we don't expect update.
(
False,
False,
False,
False,
False,
False,
1,
), # Force is off, we have a no diff, should_update_resource == false: we don't expect update.
pytest.param(True, True, True, False, False, True, id="Force, diff, local file change -> no prompt, no validation, expect update."),
pytest.param(
True, True, False, False, False, True, id="Force, diff, no local file change -> no prompt, no validation, expect update."
),
pytest.param(
True, False, False, False, False, True, id="Force, no diff, no local file change -> no prompt, no validation, expect update."
),
pytest.param(
True, False, True, False, False, True, id="Force, no diff, local file change -> no prompt, no validation, expect update."
),
pytest.param(
False, True, True, True, True, True, id="No force, diff, local file change -> expect prompt, validation, expect update."
),
pytest.param(
False, True, True, True, False, False, id="No force, diff, local file change -> expect prompt, no validation, no update."
),
pytest.param(
False, True, False, True, True, True, id="No force, diff, no local file change -> expect prompt, validation, expect update."
),
pytest.param(
False, True, False, True, False, False, id="No force, diff, no local file change -> expect prompt, no validation, no update."
),
pytest.param(
False, False, True, False, False, True, id="No force, no diff, local file change -> no prompt, no validation, expect update."
),
pytest.param(
False, False, False, False, False, False, id="No force, no diff, no local file change -> no prompt, no validation, no update."
),
],
)
def test_update_resource(
patch_click, mocker, force, diff, should_update_resource, user_validate_diff, expect_prompt, expect_update, expected_number_of_messages
):
def test_update_resource(patch_click, mocker, force, diff, local_file_changed, expect_prompt, user_validation, expect_update):
mock_updated_resource = mocker.Mock()
mock_state = mocker.Mock()
mock_resource = mocker.Mock(
get_diff_with_remote_resource=mocker.Mock(return_value=diff),
resource_name="my_resource",
local_file_changed=local_file_changed,
update=mocker.Mock(return_value=(mock_updated_resource, mock_state)),
)
update_reason = "foo"
mocker.patch.object(commands, "should_update_resource", mocker.Mock(return_value=(should_update_resource, update_reason)))
mocker.patch.object(commands, "prompt_for_diff_validation", mocker.Mock(return_value=user_validate_diff))
mocker.patch.object(commands, "prompt_for_diff_validation", mocker.Mock(return_value=user_validation))

output_messages = commands.update_resource(mock_resource, force)
commands.click.echo.assert_called_once()

if expect_prompt:
commands.prompt_for_diff_validation.assert_called_once_with("my_resource", diff)
else:
Expand All @@ -259,11 +258,9 @@ def test_update_resource(
mock_resource.update.assert_called_once()
else:
mock_resource.update.assert_not_called()
assert output_messages[0] == commands.should_update_resource.return_value[1] == update_reason
assert len(output_messages) == expected_number_of_messages
if expected_number_of_messages == 3:

if expect_update:
assert output_messages == [
commands.should_update_resource.return_value[1],
commands.click.style.return_value,
commands.click.style.return_value,
]
Expand All @@ -273,6 +270,8 @@ def test_update_resource(
mocker.call(f"💾 - New state for {mock_updated_resource.name} stored at {mock_state.path}.", fg="yellow"),
]
)
else:
assert output_messages == []


def test_find_local_configuration_files(mocker):
Expand Down

0 comments on commit 3346edc

Please sign in to comment.