Charm review PR #34
Charm review PR #34
Conversation
shipperizer
added
help wanted
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
v4 is already out. You might wanna update this.
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - name: Release charm to channel | ||
| uses: canonical/charming-actions/release-charm@2.3.0 |
| name: Promote charm | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - uses: actions/checkout@v3 |
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
I think you might wanna consider configuring Dependabot to automatically update the versions for you.
| juju deploy ./glauth-k8s_ubuntu-*-amd64.charm --resource oci-image=$(yq eval '.resources.oci-image.upstream-source' metadata.yaml) | ||
| ``` | ||
|
|
||
| ## Observability |
There was a problem hiding this comment.
Feel free to ignore this, but doesn't README seem more suitable for this part?
| def test_when_missing_database_relation( | ||
| self, | ||
| harness: Harness, | ||
| certificates_relation: int, |
| def test_when_missing_certificates_relation( | ||
| self, | ||
| harness: Harness, | ||
| database_relation: int, |
There was a problem hiding this comment.
There's a lot of unused args all over the file. Looks like they can all be safely removed.
|
|
||
| assert isinstance(harness.model.unit.status, BlockedStatus) | ||
|
|
||
| def test_when_tls_certificates_not_exist( |
There was a problem hiding this comment.
It's a personal preference (and something to think about), but I'm a great fan of using given-when-then in the tests. This can go either in the name of the test or a docstring. For someone not involved in the charm design process (like me :) ) it makes it easier to understand the expected behavior and hence provide a better review ;)
| apps=[GLAUTH_APP], | ||
| status="active", | ||
| timeout=1000, | ||
| ) |
There was a problem hiding this comment.
Shouldn't we wait for the target_unit_num specifically?
|
|
||
| import textwrap | ||
|
|
||
| ANY_CHARM = textwrap.dedent( |
There was a problem hiding this comment.
What's the reason for having this instead of a minimal but real charm stored under the integration dir? This seems pretty complicated. Charm code is here, overwrites in the test_charm.py, charm name in the conftest.py.
PR for facilitating charm review process
DO NOT MERGE