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

🐙 octavia-cli: make update logic clearer #11386

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Mar 24, 2022

What

The update_resource and should_update_resources function deserved a clearer and more tested logic to make sure all update cases are covered.

This closes #11234 as the messages displayed to users are now properly ordered and happening at the right execution time.

How

In octavia-cli/octavia_cli/apply/commands.py:

  • Change should_update_resource to make it explicitely handle the case when user does not validate the changes or when a local file changed. I also changed the message to make these clearer.
  • Refactor update_resource to first get user validation if needed (in case of a diff) and then call should_update_resource to decide if an update is required.

In octavia-cli/unit_tests/test_apply/test_commands.py:

  • Improve testing of update_resource and should_update_resource by making the test cases more readable and disabling the mock of should_update_resource used in update_resource to make sure both function work well together.

Reminder of the update logic we want:

We want to update a resource in these multiple contexts:

  • The force mode is on
  • A diff was detected between the local and remote configuration and the user validate the changes
  • No diff was detected but a change in the local file (or env var value) was detected thanks to the hash comparison. This allow detection of a secret value update.

🚨 User Impact 🚨

  • Better update messages

@alafanechere alafanechere marked this pull request as ready for review March 24, 2022 10:06
@alafanechere alafanechere added this to the octavia-cli-alpha milestone Mar 24, 2022
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had one tiny nit about a typo, otherwise this looks like a good change that improves the organization of this logic, nice!

@alafanechere
Copy link
Contributor Author

Just had one tiny nit about a typo, otherwise this looks like a good change that improves the organization of this logic, nice!

Thank you for spotting this typo and reading the tests 😄 !

@alafanechere alafanechere merged commit 3346edc into master Mar 25, 2022
@alafanechere alafanechere deleted the augustin/octavia-cli/fix-update-resource branch March 25, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 octavia-cli: update message displayed event if there's no update
2 participants