-
Notifications
You must be signed in to change notification settings - Fork 9
Add refresh v3 draft implementation #411
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
Does not include migration for refreshing from v2 (currently on stable) This PR branch only supports refreshing to/from mysql-router-k8s charm code with refresh v3
b4ac782
to
6a76b3f
Compare
You can test the refresh with these charmhub branches (built on #412):
or modify the branch locally & re-pack the charm |
# is pending review. | ||
python3 -c 'import pathlib; import shutil; import subprocess; git_hash=subprocess.run(["git", "describe", "--always", "--dirty"], capture_output=True, check=True, encoding="utf-8").stdout; file = pathlib.Path("charm_version"); shutil.copy(file, pathlib.Path("charm_version.backup")); version = file.read_text().strip(); file.write_text(f"{version}+{git_hash}")' | ||
|
||
# TODO: set charm version in refresh_versions.toml |
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.
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.
added to spec sync sheet
# autogenerated | ||
charm = "8.0/1.0.0" |
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.
once https://docs.google.com/document/d/1Jv1jhWLl8ejK3iJn7Q3VbCIM9GIhp8926bgXpdtx-Sg/edit?tab=t.0 is approved, this part will not be included in the git respository & will be auto-generated at pack time using the git tag
src/charm.py
Outdated
# TODO: check workload version—prevent downgrade? | ||
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.
do we want to warn incompatible if user tries to downgrade router?
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.
If it's just a warning, yes. No otherwise
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.
this is specifically what would happen: https://github.com/canonical/charm-refresh/blob/draft/docs/requirements_and_user_experience.md#machines-only-if-refresh-is-incompatible
# is pending review. | ||
python3 -c 'import pathlib; import shutil; import subprocess; git_hash=subprocess.run(["git", "describe", "--always", "--dirty"], capture_output=True, check=True, encoding="utf-8").stdout; file = pathlib.Path("charm_version"); shutil.copy(file, pathlib.Path("charm_version.backup")); version = file.read_text().strip(); file.write_text(f"{version}+{git_hash}")' | ||
|
||
# TODO: set charm version in refresh_versions.toml |
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.
added to spec sync sheet
@@ -14,6 +14,7 @@ tenacity = "^8.5.0" | |||
jinja2 = "^3.1.4" | |||
poetry-core = "^1.9.0" | |||
requests = "^2.32.3" | |||
charm-refresh = {git = "https://github.com/canonical/charm-refresh", rev = "draft"} |
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.
what are you waiting for before tagging this?
# only in machine charms | ||
if self._ha_cluster: | ||
statuses.append(self._ha_cluster.get_unit_juju_status()) | ||
statuses.append(workload_status) | ||
if status := self._ha_cluster.get_unit_juju_status(): |
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 append HA status only when workload_.status
?
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.
I don't see the dependency on workload_.status
@carlcsaposs-canonical What's the plan for integration testing the refresh? |
use existing integration tests in the future, I think we could improve the coverage of the refresh tests, but I don't think we have enough time allocated to do that now |
2103190
to
b8b5835
Compare
b8b5835
to
b8ce54f
Compare
Does not include migration for refreshing from v2 (currently on stable)
This PR branch only supports refreshing to/from mysql-router-k8s charm code with refresh v3
Uses
charm-refresh
Python package: https://github.com/canonical/charm-refresh/tree/draft