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

[DPE-5711] Add warnings to destructive actions #188

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

sinclert-canonical
Copy link
Contributor

This PR adds warning level messages when a destructive action is performed. The definition of destructive seems to be associated with data loss, which I think it can only happen, from user action, whenever they force an upgrade when the charm has already detected its lack of compatibility.

Additional info

  • Paulo confirmed that this ticket was created to familiarised myself with the codebases.
  • Alex referenced this PostgreSQL operator PR as the reason behind the need for warnings.

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

BTW, I would also warn:

15:17:20 ✔ taurus:(main)~/canonical/mysql-router-operator$ rg rmtree
src/container.py
50:    def rmtree(self):

src/snap.py
153:    def rmtree(self):
154:        shutil.rmtree(self)

lib/charms/tempo_coordinator_k8s/v0/charm_tracing.py
220:                shutil.rmtree(path)

src/workload.py
151:        self._container.router_config_directory.rmtree()
153:        self._router_data_directory.rmtree()

tests/unit/conftest.py
111:    monkeypatch.setattr("snap._Path.rmtree", lambda *args, **kwargs: None)
15:17:21 ✔ taurus:(main)~/canonical/mysql-router-operator$ 

@sinclert-canonical
Copy link
Contributor Author

My interpretation was that I should only warn about potential data loss / some destruction (from the JIRA ticket). To the best of my understanding, the Workload._disable_router method only removes access, but it does not produce data-loss. This method is even called during "normal" actions such as upgrade (reference).

I am open to also warn about those cases, if the criteria gets extended to cover both data-loss and access-loss scenarios. Do you want me to?

@paulomach
Copy link
Contributor

My interpretation was that I should only warn about potential data loss / some destruction (from the JIRA ticket). To the best of my understanding, the Workload._disable_router method only removes access, but it does not produce data-loss. This method is even called during "normal" actions such as upgrade (reference).

I am open to also warn about those cases, if the criteria gets extended to cover both data-loss and access-loss scenarios. Do you want me to?

Besides test and lib code, Alex do have a point. Even though the method is called on "normal" handling, we cannot assume it will not be called unexpectedly due some bug, and that's exactly when we most want to have it logged.

@sinclert-canonical sinclert-canonical force-pushed the sinclert/force-upgrade-warnings branch from 48a44fe to dc2146d Compare November 28, 2024 15:17
@sinclert-canonical
Copy link
Contributor Author

Added info messages to Workload._diable_router method.

@sinclert-canonical sinclert-canonical merged commit 950a9ea into main Nov 28, 2024
54 of 58 checks passed
@sinclert-canonical sinclert-canonical deleted the sinclert/force-upgrade-warnings branch November 28, 2024 16:20
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.

4 participants