Merge ~barryprice/charm-k8s-discourse/+git/charm-k8s-discourse:v2.8.0.beta7-20.04 into charm-k8s-discourse:v2.8.0.beta7-20.04

Proposed by Barry Price
Status: Merged
Approved by: Barry Price
Approved revision: 902dfd48f34c356618ef9da905fb2ca9ec4a119f
Merge reported by: Barry Price
Merged at revision: 902dfd48f34c356618ef9da905fb2ca9ec4a119f
Proposed branch: ~barryprice/charm-k8s-discourse/+git/charm-k8s-discourse:v2.8.0.beta7-20.04
Merge into: charm-k8s-discourse:v2.8.0.beta7-20.04
Diff against target: 155 lines (+34/-18)
2 files modified
lib/charms/redis_k8s/v0/redis.py (+33/-14)
tox.ini (+1/-4)
Reviewer Review Type Date Requested Status
Tom Haddon Approve
prod-jenkaas-is (community) continuous-integration Needs Fixing
Canonical IS Reviewers Pending
Review via email: mp+411007@code.launchpad.net

Commit message

Run charmcraft fetch-lib to update our redis_k8s library, and update tox.ini to use the correct PYTHONPATH

To post a comment you must log in.
Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :

A CI job is currently in progress. A follow up comment will be added when it completes.

Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tom Haddon (mthaddon) wrote :

A comment inline about the path being controllable via tox.

Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :

A CI job is currently in progress. A follow up comment will be added when it completes.

Revision history for this message
Barry Price (barryprice) wrote :

Hmm I think I'm missing something then, tox.ini already has:

[testenv]
basepython = python3
setenv =
  PYTHONPATH = {toxinidir}/build/lib:{toxinidir}/build/venv

Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tom Haddon (mthaddon) wrote :

I think there's been a change in charmcraft which means the "build" directory is no longer used as before (it's now used in a way similar to snapcraft).

As a result I think you'll want something like this https://pastebin.ubuntu.com/p/kkx5d6Pgf2/ - essentially just adding `{toxinidir}/lib` to the testenv path. Fwiw, Jon Seager's teams will be standardising on a tox.ini similar to this https://github.com/jnsgruk/charm-kubernetes-dashboard/blob/main/tox.ini. I'm not saying we should change to this exactly, but it shows `{toxinidir}/lib` as part of testenv.

Revision history for this message
Barry Price (barryprice) wrote :

Thanks, that works locally - have pushed another revision.

Removing the existing directories in PYTHONPATH doesn't seem to cause any problems either, so if those are historical I think they can go too.

Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :

A CI job is currently in progress. A follow up comment will be added when it completes.

Revision history for this message
prod-jenkaas-is (prod-jenkaas-is) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Tom Haddon (mthaddon) wrote :

LGTM, thx

review: Approve
Revision history for this message
Barry Price (barryprice) wrote :

Mergebot can't land this because of LP:1949763 and LP:1949764.

Marking it WIP until those are addressed, then we can use this to test the fixes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/charms/redis_k8s/v0/redis.py b/lib/charms/redis_k8s/v0/redis.py
2index 70787b6..5360ce0 100644
3--- a/lib/charms/redis_k8s/v0/redis.py
4+++ b/lib/charms/redis_k8s/v0/redis.py
5@@ -6,15 +6,29 @@ redis interface.
6 Import `RedisRequires` in your charm by adding the following to `src/charm.py`:
7 ```
8 from charms.redis_k8s.v0.redis import RedisRequires
9-
10-# In your charm's `__init__` method.
11+```
12+And then in your charm's `__init__` method:
13+```
14+# Make sure you set redis_relation in StoredState. Assuming you refer to this
15+# as `self._stored`:
16+self._stored.set_default(redis_relation={})
17 self.redis = RedisRequires(self, self._stored)
18 ```
19-And then wherever you need to reference the relation data:
20+And then wherever you need to reference the relation data it will be available
21+via StoredState:
22 ```
23+redis_hosts = []
24 for redis_unit in self._stored.redis_relation:
25- redis_host = self._stored.redis_relation[redis_unit]["hostname"]
26- redis_port = self._stored.redis_relation[redis_unit]["port"]
27+ redis_hosts.append({
28+ "redis-host": self._stored.redis_relation[redis_unit]["hostname"],
29+ "redis-port": self._stored.redis_relation[redis_unit]["port"],
30+ })
31+```
32+You will also need to add the following to `metadata.yaml`:
33+```
34+requires:
35+ redis:
36+ interface: redis
37 ```
38 """
39 import logging
40@@ -22,30 +36,31 @@ import logging
41 from ops.charm import CharmEvents
42 from ops.framework import EventBase, EventSource, Object
43
44-# The unique Charmhub library identifier, never change it
45+# The unique Charmhub library identifier, never change it.
46 LIBID = "fe18a608cec5465fa5153e419abcad7b"
47
48-# Increment this major API version when introducing breaking changes
49+# Increment this major API version when introducing breaking changes.
50 LIBAPI = 0
51
52 # Increment this PATCH version before using `charmcraft publish-lib` or reset
53-# to 0 if you are raising the major API version
54-LIBPATCH = 1
55+# to 0 if you are raising the major API version.
56+LIBPATCH = 2
57
58 logger = logging.getLogger(__name__)
59
60
61 class RedisRelationUpdatedEvent(EventBase):
62- pass
63+ """An event for the redis relation having been updated."""
64
65
66 class RedisRelationCharmEvents(CharmEvents):
67+ """A class to carry custom charm events so requires can react to relation changes."""
68 redis_relation_updated = EventSource(RedisRelationUpdatedEvent)
69
70
71 class RedisRequires(Object):
72 def __init__(self, charm, _stored):
73- # Define a constructor that takes the charm and it's StoredState
74+ """A class implementing the redis requires relation."""
75 super().__init__(charm, "redis")
76 self.framework.observe(charm.on.redis_relation_changed, self._on_relation_changed)
77 self.framework.observe(charm.on.redis_relation_broken, self._on_relation_broken)
78@@ -53,19 +68,20 @@ class RedisRequires(Object):
79 self.charm = charm
80
81 def _on_relation_changed(self, event):
82+ """Handle the relation changed event."""
83 if not event.unit:
84 return
85
86 hostname = event.relation.data[event.unit].get("hostname")
87 port = event.relation.data[event.unit].get("port")
88- # Store some data from the relation in local state
89 self._stored.redis_relation[event.relation.id] = {"hostname": hostname, "port": port}
90
91 # Trigger an event that our charm can react to.
92 self.charm.on.redis_relation_updated.emit()
93
94 def _on_relation_broken(self, event):
95- # Remove the unit data from local state
96+ """Handle the relation broken event."""
97+ # Remove the unit data from local state.
98 self._stored.redis_relation.pop(event.relation.id, None)
99
100 # Trigger an event that our charm can react to.
101@@ -74,23 +90,26 @@ class RedisRequires(Object):
102
103 class RedisProvides(Object):
104 def __init__(self, charm, port):
105+ """A class implementing the redis provides relation."""
106 super().__init__(charm, "redis")
107 self.framework.observe(charm.on.redis_relation_changed, self._on_relation_changed)
108 self._port = port
109
110 def _on_relation_changed(self, event):
111+ """Handle the relation changed event."""
112 if not self.model.unit.is_leader():
113 logger.debug("Relation changes ignored by non-leader")
114 return
115
116 event.relation.data[self.model.unit]['hostname'] = str(self._bind_address(event))
117 event.relation.data[self.model.unit]['port'] = str(self._port)
118- # The reactive Redis charm exposes also 'password'. When tackling
119+ # The reactive Redis charm also exposes 'password'. When tackling
120 # https://github.com/canonical/redis-operator/issues/7 add 'password'
121 # field so that it matches the exposed interface information from it.
122 # event.relation.data[self.unit]['password'] = ''
123
124 def _bind_address(self, event):
125+ """Convenience function for getting the unit address."""
126 relation = self.model.get_relation(event.relation.name, event.relation.id)
127 if address := self.model.get_binding(relation).network.bind_address:
128 return address
129diff --git a/tox.ini b/tox.ini
130index d48b030..857a545 100644
131--- a/tox.ini
132+++ b/tox.ini
133@@ -5,8 +5,6 @@ skip_missing_interpreters = True
134
135 [testenv]
136 basepython = python3
137-setenv =
138- PYTHONPATH = {toxinidir}/build/lib:{toxinidir}/build/venv
139
140 [testenv:unit]
141 commands =
142@@ -15,7 +13,7 @@ commands =
143 deps = -r{toxinidir}/tests/unit/requirements.txt
144 -r{toxinidir}/requirements.txt
145 setenv =
146- PYTHONPATH={toxinidir}/src:{toxinidir}/build/lib:{toxinidir}/build/venv
147+ PYTHONPATH={toxinidir}/src:{toxinidir}/lib
148 TZ=UTC
149
150 [testenv:black]
151@@ -32,4 +30,3 @@ exclude = .git, __pycache__, .tox,
152 ignore = E231
153 max-line-length = 120
154 max-complexity = 10
155-

Subscribers

People subscribed via source and target branches