Add refresh v3 implementation #241
Conversation
carlcsaposs-canonical
force-pushed
the
refresh-v3-draft
branch
23 times, most recently
from
7d294ef to
d7d94fc
Compare
carlcsaposs-canonical
force-pushed
the
refresh-v3-draft
branch
from
d7d94fc to
db4ec41
Compare
|
You can test the refresh with these charmhub branches (built on #242):
or modify the branch locally & re-pack the charm |
carlcsaposs-canonical
requested review from
paulomach,
taurus-forever,
shayancanonical and
sinclert-canonical
carlcsaposs-canonical
force-pushed
the
refresh-v3-draft
branch
from
e37f480 to
16a3cbb
Compare
|
all tests passed on 16a3cbb: https://github.com/canonical/mysql-router-operator/actions/runs/16269250664?pr=241 ef71408 reverts changes for testing that needed to be reverted before release |
carlcsaposs-canonical
requested review from
paulomach,
taurus-forever and
sinclert-canonical
There was a problem hiding this comment.
🥳
P.S. @carlcsaposs-canonical please do not forget to remove this after the merge.
| def refresh_snap( | ||
| self, *, snap_name: str, snap_revision: str, refresh: charm_refresh.Machines | ||
| ) -> None: | ||
| # TODO: issue on relation-broken event since event not passed? mitigated by regular event handler? |
There was a problem hiding this comment.
Why this TODO? Are we waiting on a Juju / Ops framework bug to get fixed? It would be nice to know why this is an issue, and when could we consider it resolved.
There was a problem hiding this comment.
This is a potential edge case that we might need to address later, although it is unlikely to happen in production (would require the user to remove the relation while the charm is refreshing the snap)
Left the comment in case that edge case happens to become a real issue
The robustness of refresh has significantly improved from v2 in this PR, don't think this minor edge case should block
| str(content), | ||
| ) | ||
| charm_zip.writestr("src/snap.py", new_snap_content) | ||
| charm_zip.writestr("refresh_versions.toml", tomli_w.dumps(versions)) | ||
|
|
||
|
|
||
| def create_invalid_upgrade_charm(charm_file: typing.Union[str, pathlib.Path]) -> None: |
There was a problem hiding this comment.
Generic comment for the whole file
Should we replace the word upgrade by the word refresh throughout this file, in addition to the file name? Not sure if we want to continue considering this functionality as a new type of upgrade, or whether should we start calling it refresh everywhere 🤔
There was a problem hiding this comment.
Don't think it's too important since it doesn't affect UX
Everything that's exposed to the user has been changed to "refresh"
| @@ -6,13 +6,14 @@ package-mode = false | |||
| requires-poetry = ">=2.0.0" | |||
|
|
|||
| [tool.poetry.dependencies] | |||
| python = "^3.8.6" # ^3.8.6 required by juju | |||
| python = "^3.10" | |||
There was a problem hiding this comment.
Is this version bump related to the recent drop of support for Ubuntu 20.04 base? If so, could we mention the package that is making us stay in Python 3.10 (similarly to how we had it before)?
Ideally we will make this bump on a separate PR, where all the # TODO python3.10 comments are properly addressed (see GitHub search).
There was a problem hiding this comment.
We're staying in 3.10 since the base is 22.04. The comment was when we needed a python version higher than what's required by the base
Uses
charm-refreshPython package: https://github.com/canonical/charm-refresh