-
Notifications
You must be signed in to change notification settings - Fork 18
[DPE-8916] Use mysql-shell-client package #727
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
base: main
Are you sure you want to change the base?
Conversation
770c1c2 to
045ea49
Compare
paulomach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, I like the direction this is going.
out of a couple comments just one is a blocker (about admin port), otherise lgtm
| @@ -723,12 +731,14 @@ def cluster_initialized(self) -> bool: | |||
| if not self.app_peer_data.get("cluster-name"): | |||
| return False | |||
|
|
|||
| if self.unit_initialized(): | |||
| return True | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebase on #729 merge
lib/charms/mysql/v0/mysql.py
Outdated
| if host and ":" in host: | ||
| # strip port from address | ||
| host = host.split(":")[0] | ||
| def _build_cluster_tcp_executor(self, host: str, port: int = 3306) -> BaseExecutor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use ADMIN_PORT for cluster executor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real reason apart from: it has not been used for the clusteradmin user so far. Do you recall any reason why this was not the case? Maybe the cluster-admin user was not present on old versions of the charm? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was, but we were not doing much with it. In any case, we should use admin port, we don't want some cluster action to fail because we cannot connect to the a host
| self.stop_mysql_exporter() | ||
| self.connect_mysql_exporter() | ||
|
|
||
| def _run_mysqlsh_script( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removal feels good :)
6dc8195 to
8a51924
Compare
7be5170 to
f4c00a9
Compare
f4c00a9 to
c443f86
Compare
163237c to
f56cff9
Compare
f56cff9 to
608ef38
Compare
608ef38 to
f529cf5
Compare
f529cf5 to
44c9dd5
Compare
44c9dd5 to
4f3d6e4
Compare
This PR removes a whole horizontal slice of the charm code, to rely on mysql-shell-client package.
Exceptions to this extraction are:
Highlights:
Removed.lock_instanceargument fromadd_instance_to_clustercharm lib functionRemoved.lock_instanceargument fromremove_instance_to_clustercharm lib functionReview strategy
Given the amount of changes proposed on this PR, I recommend reviewing commit by commit. When reaching 3d32f6f, please compared side-by-side:
mysqlcharm lib.mysqlcharm lib.