Merge lp:~reedobrien/charmworld/es-migration into lp:charmworld
- es-migration
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Reed O'Brien |
Approved revision: | 517 |
Merged at revision: | 512 |
Proposed branch: | lp:~reedobrien/charmworld/es-migration |
Merge into: | lp:charmworld |
Diff against target: |
370 lines (+281/-41) 5 files modified
charmworld/migrations/versions/027_use_ngrams.py (+35/-0) charmworld/migrations/versions/tests/__init__.py (+1/-0) charmworld/migrations/versions/tests/data/migration_027.py (+166/-0) charmworld/migrations/versions/tests/test_migrations.py (+79/-4) charmworld/search.py (+0/-37) |
To merge this branch: | bzr merge lp:~reedobrien/charmworld/es-migration |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Gui Bot | continuous-integration | Approve | |
Brad Crittenden (community) | Approve | ||
Review via email:
|
Commit message
This makes the ngram work migrate cleanly.
Description of the change
Hopefully this makes the ngram work migrate cleanly.
QA:
Go back to r508
$ bzr revert -r 508
Drop the version and delte the index:
$ mongo juju --eval "db.migration_
$ curl -XDELETE http://
Create the r508 index:
$ bin/es-update
Setup the migration system and update to r508 version (026)
$ bin/migrations current
$ bin/migrations prepare-upgrade -i
$ bin/migrations upgrade
$ bin/migrations latest
Reindex the stuff...without ngrams
$ bin/sync-index
Go back to tip (r515)
$ bzr revert
Do the migration...
$ bin/migrations latest
$ bin/migrations current
$ bin/es-update
$ bin/migrations upgrade
Verify it worked
$ curl localhost:
$ curl localhost:
$ curl localhost:
Make sure the tests all pass...
$ make test
- 516. By Reed O'Brien
-
Remove unused bits of the exists in index helper method.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
Oh, and do see the in-line comments!
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brad Crittenden (bac) wrote : | # |
Actually you should pass the kind in the exists method:
def exists_
client = self.index_client
return client.get(id_, kind) is not None
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Reed O'Brien (reedobrien) wrote : | # |
> Reed this branch looks great, modulo some stylistic issues.
>
> The QA steps you provide are fantastic. The last two steps do make
> assumptions about what charms have been ingested locally.
Thanks. I thought I had put some ingest bits at the top. I missed that I guess -- nearly fantastic...
> I've marked the branch as approved, which means you are free to land after
> making (or discussing) the suggested changes. Had I wanted to see the changes
> made before you land I would've used 'needs fixing' but I don't, so I didn't.
I'll resubmit for sanity check.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Reed O'Brien (reedobrien) wrote : | # |
> Oh, and do see the in-line comments!
Waiting for them to show here. But will work from email until they do.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Reed O'Brien (reedobrien) wrote : | # |
> Actually you should pass the kind in the exists method:
>
> def exists_
> client = self.index_client
> return client.get(id_, kind) is not None
This shrank to:
def exists_
return self.index_
in this test I am only inserting a charm, so adding a specifier seems like noise. If adding a charm let's you find something else, there is a problem and that should be tested elsewhere, IMO. Unless we are aiming to test that bundles upgrade as well, which AFAIU we are not.
- 517. By Reed O'Brien
-
Assuages stylistic deviations.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Reed O'Brien (reedobrien) wrote : | # |
On Tue, May 20, 2014 at 9:04 AM, Brad Crittenden <email address hidden> wrote:
> Review: Approve bac
>
> Reed this branch looks great, modulo some stylistic issues.
>
> The QA steps you provide are fantastic. The last two steps do make
> assumptions about what charms have been ingested locally.
>
> Note this review is based on the version of your code as of 8:00am, not
> the branch you subsequently pushed.
>
> I've marked the branch as approved, which means you are free to land after
> making (or discussing) the suggested changes. Had I wanted to see the
> changes made before you land I would've used 'needs fixing' but I don't, so
> I didn't.
>
> Thanks for persevering and getting this in so we can do the deploy.
>
> Inline comments:
>
>
> > + except IndexMissing:
> > + # for whatever reason it is updated already
> > + pass
>
> Remember to Capitalize and punctuate comments.
>
Done.
> > --- charmworld/
> 00:00:00 +0000
> > +++ charmworld/
> 01:08:32 +0000
> > @@ -0,0 +1,1 @@
> > +# a package
> >
>
> I'd actually prefer to see a totally empty file than just this comment.
> See
>
Emptied. This is a vestigial habit from some old VCS that wouldn't commit
empty files and directories.
I can't even remember what that was... Removed.
> > +old_charm_mapping = {"charm": {
> > + "type": "object",
>
> Thanks for figuring out the old mapping structure to provide the test.
> Makes for much more thorough testing.
>
Getting the mapping was easy. Formatting it and converting to python were
less so.
> > === modified file
> 'charmworld/
> > --- charmworld/
> 20:36:12 +0000
> > +
> > +from charmworld.search import CHARM
> > +
>
> If you import BUNDLE too you can use them instead of strings, like at line
> 304.
>
I am actually not importing CHARM at this point either. See other comment
here.
>
> > +from pyelasticsearch
> > + ElasticHttpError,
> > + ElasticHttpNotF
> > +)
> >
> > import logging
> > import mock
> > @@ -126,3 +138,79 @@
> > def test_exodus_
> > """Ensure that deploy-from-scratch does not violate exodus
> rules."""
> > self.versions.
> > +
> > +
> > +class TestUseNgrams02
> > +
> > + version = 27
> > + module_name = '027_use_ngrams.py'
> > +
> > + def setUp(self):
> > + super(TestUseNg
> > + self.use_
> > + self.ds = DataStore(self.db)
> > + self.cs = CharmSource(
>
> If this were long-lived code I'd suggest more explicit names. We tend to
> the follow the Python guide that code is to be read many more times than
> written so more readable names are better at the sake of brevity.
>
Embiggened.
> > +
> > + def exists_
> > + client = self.index_client
> > + if kind == CHARM:
> > + return client.get(id_) is not None
> > +...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Juju Gui Bot (juju-gui-bot) : | # |
Preview Diff
1 | === added file 'charmworld/migrations/versions/027_use_ngrams.py' | |||
2 | --- charmworld/migrations/versions/027_use_ngrams.py 1970-01-01 00:00:00 +0000 | |||
3 | +++ charmworld/migrations/versions/027_use_ngrams.py 2014-05-20 14:13:54 +0000 | |||
4 | @@ -0,0 +1,35 @@ | |||
5 | 1 | # use ngrams | ||
6 | 2 | import logging | ||
7 | 3 | |||
8 | 4 | from charmworld.models import CharmSource | ||
9 | 5 | |||
10 | 6 | from charmworld.search import ( | ||
11 | 7 | IndexMissing, | ||
12 | 8 | update, | ||
13 | 9 | ) | ||
14 | 10 | |||
15 | 11 | |||
16 | 12 | def upgrade(db, index_client): | ||
17 | 13 | """db is the pymongo db instance for our datastore. Charms are in db.charms | ||
18 | 14 | for instance. | ||
19 | 15 | |||
20 | 16 | index_client is the ElasticSearch client. | ||
21 | 17 | """ | ||
22 | 18 | log = logging.getLogger('migration_027') | ||
23 | 19 | log.warning('Starting') | ||
24 | 20 | charm_source = CharmSource(db, index_client) | ||
25 | 21 | try: | ||
26 | 22 | index_client.delete_index() | ||
27 | 23 | log.warning("deleted index") | ||
28 | 24 | except IndexMissing: | ||
29 | 25 | # It is already updated. | ||
30 | 26 | pass | ||
31 | 27 | log.warning("Index already deleted") | ||
32 | 28 | update(index_client) | ||
33 | 29 | cids = charm_source.sync_index() | ||
34 | 30 | while True: | ||
35 | 31 | try: | ||
36 | 32 | next(cids) | ||
37 | 33 | except StopIteration: | ||
38 | 34 | break | ||
39 | 35 | log.warning("Index updated") | ||
40 | 0 | 36 | ||
41 | === added file 'charmworld/migrations/versions/tests/__init__.py' | |||
42 | --- charmworld/migrations/versions/tests/__init__.py 1970-01-01 00:00:00 +0000 | |||
43 | +++ charmworld/migrations/versions/tests/__init__.py 2014-05-20 14:13:54 +0000 | |||
44 | @@ -0,0 +1,1 @@ | |||
45 | 1 | # a package | ||
46 | 0 | 2 | ||
47 | === added directory 'charmworld/migrations/versions/tests/data' | |||
48 | === added file 'charmworld/migrations/versions/tests/data/__init__.py' | |||
49 | === added file 'charmworld/migrations/versions/tests/data/migration_027.py' | |||
50 | --- charmworld/migrations/versions/tests/data/migration_027.py 1970-01-01 00:00:00 +0000 | |||
51 | +++ charmworld/migrations/versions/tests/data/migration_027.py 2014-05-20 14:13:54 +0000 | |||
52 | @@ -0,0 +1,166 @@ | |||
53 | 1 | old_charm_mapping = {"charm": { | ||
54 | 2 | "type": "object", | ||
55 | 3 | "dynamic": "false", | ||
56 | 4 | "properties": { | ||
57 | 5 | "data": { | ||
58 | 6 | "properties": { | ||
59 | 7 | "_id": { | ||
60 | 8 | "type": "string", | ||
61 | 9 | "index": "not_analyzed", | ||
62 | 10 | "omit_norms": True, | ||
63 | 11 | "index_options": "docs" | ||
64 | 12 | }, | ||
65 | 13 | "categories": { | ||
66 | 14 | "type": "string", | ||
67 | 15 | "index": "not_analyzed", | ||
68 | 16 | "omit_norms": True, | ||
69 | 17 | "index_options": "docs" | ||
70 | 18 | }, | ||
71 | 19 | "config": { | ||
72 | 20 | "properties": { | ||
73 | 21 | "options": { | ||
74 | 22 | "properties": { | ||
75 | 23 | "default": { | ||
76 | 24 | "type": "string"}, | ||
77 | 25 | "description": { | ||
78 | 26 | "type": "string"} | ||
79 | 27 | } | ||
80 | 28 | } | ||
81 | 29 | } | ||
82 | 30 | }, | ||
83 | 31 | "date_created": { | ||
84 | 32 | "type": "date", | ||
85 | 33 | "format": "dateOptionalTime" | ||
86 | 34 | }, | ||
87 | 35 | "description": {"type": "string"}, | ||
88 | 36 | "downloads": {"type": "long"}, | ||
89 | 37 | "downloads_in_past_30_days": {"type": "long"}, | ||
90 | 38 | "downloads_in_past_7_days": {"type": "long"}, | ||
91 | 39 | "downloads_in_past_half_year": {"type": "long"}, | ||
92 | 40 | "i_provides": { | ||
93 | 41 | "type": "string", | ||
94 | 42 | "index": "not_analyzed", | ||
95 | 43 | "omit_norms": True, | ||
96 | 44 | "index_options": "docs" | ||
97 | 45 | }, | ||
98 | 46 | "i_requires": { | ||
99 | 47 | "type": "string", | ||
100 | 48 | "index": "not_analyzed", | ||
101 | 49 | "omit_norms": True, | ||
102 | 50 | "index_options": "docs" | ||
103 | 51 | }, | ||
104 | 52 | "is_featured": {"type": "boolean"}, | ||
105 | 53 | "name": { | ||
106 | 54 | "type": "string", | ||
107 | 55 | "index": "not_analyzed", | ||
108 | 56 | "omit_norms": True, | ||
109 | 57 | "index_options": "docs" | ||
110 | 58 | }, | ||
111 | 59 | "owner": { | ||
112 | 60 | "type": "string", | ||
113 | 61 | "index": "not_analyzed", "omit_norms": True, | ||
114 | 62 | "index_options": "docs" | ||
115 | 63 | }, | ||
116 | 64 | "series": { | ||
117 | 65 | "type": "string", | ||
118 | 66 | "index": "not_analyzed", | ||
119 | 67 | "omit_norms": True, | ||
120 | 68 | "index_options": "docs" | ||
121 | 69 | }, | ||
122 | 70 | "store_data": { | ||
123 | 71 | "properties": { | ||
124 | 72 | "errors": {"type": "string"} | ||
125 | 73 | } | ||
126 | 74 | }, | ||
127 | 75 | "store_url": { | ||
128 | 76 | "type": "string", | ||
129 | 77 | "index": "not_analyzed", | ||
130 | 78 | "omit_norms": True, | ||
131 | 79 | "index_options": "docs" | ||
132 | 80 | }, | ||
133 | 81 | "summary": {"type": "string"} | ||
134 | 82 | } | ||
135 | 83 | }, | ||
136 | 84 | "provides": { | ||
137 | 85 | "dynamic": "false", | ||
138 | 86 | "properties": { | ||
139 | 87 | "interface": { | ||
140 | 88 | "type": "string", | ||
141 | 89 | "index": "not_analyzed", | ||
142 | 90 | "omit_norms": True, | ||
143 | 91 | "index_options": "docs" | ||
144 | 92 | }, | ||
145 | 93 | "relation_name": { | ||
146 | 94 | "type": "string", | ||
147 | 95 | "index": "not_analyzed", | ||
148 | 96 | "omit_norms": True, | ||
149 | 97 | "index_options": "docs" | ||
150 | 98 | }, | ||
151 | 99 | "scope": { | ||
152 | 100 | "type": "string", | ||
153 | 101 | "index": "not_analyzed", | ||
154 | 102 | "omit_norms": True, | ||
155 | 103 | "index_options": "docs" | ||
156 | 104 | } | ||
157 | 105 | } | ||
158 | 106 | }, | ||
159 | 107 | "requires": { | ||
160 | 108 | "dynamic": "false", | ||
161 | 109 | "properties": { | ||
162 | 110 | "interface": { | ||
163 | 111 | "type": "string", | ||
164 | 112 | "index": "not_analyzed", | ||
165 | 113 | "omit_norms": True, | ||
166 | 114 | "index_options": "docs" | ||
167 | 115 | }, | ||
168 | 116 | "relation_name": { | ||
169 | 117 | "type": "string", | ||
170 | 118 | "index": "not_analyzed", | ||
171 | 119 | "omit_norms": True, | ||
172 | 120 | "index_options": "docs" | ||
173 | 121 | }, | ||
174 | 122 | "scope": { | ||
175 | 123 | "type": "string", | ||
176 | 124 | "index": "not_analyzed", | ||
177 | 125 | "omit_norms": True, | ||
178 | 126 | "index_options": "docs" | ||
179 | 127 | } | ||
180 | 128 | } | ||
181 | 129 | } | ||
182 | 130 | } | ||
183 | 131 | }} | ||
184 | 132 | |||
185 | 133 | old_bundle_mapping = {"bundle": { | ||
186 | 134 | "type": "object", | ||
187 | 135 | "dynamic": "false", | ||
188 | 136 | "properties": { | ||
189 | 137 | "data": { | ||
190 | 138 | "properties": { | ||
191 | 139 | "name": { | ||
192 | 140 | "type": "string", | ||
193 | 141 | "index": "not_analyzed", | ||
194 | 142 | "omit_norms": True, | ||
195 | 143 | "index_options": "docs" | ||
196 | 144 | }, | ||
197 | 145 | "owner": { | ||
198 | 146 | "type": "string", | ||
199 | 147 | "index": "not_analyzed", | ||
200 | 148 | "omit_norms": True, | ||
201 | 149 | "index_options": "docs" | ||
202 | 150 | }, | ||
203 | 151 | "series": { | ||
204 | 152 | "type": "string", | ||
205 | 153 | "index": "not_analyzed", | ||
206 | 154 | "omit_norms": True, | ||
207 | 155 | "index_options": "docs" | ||
208 | 156 | }, | ||
209 | 157 | "title": { | ||
210 | 158 | "type": "string", | ||
211 | 159 | "index": "not_analyzed", | ||
212 | 160 | "omit_norms": True, | ||
213 | 161 | "index_options": "docs" | ||
214 | 162 | } | ||
215 | 163 | } | ||
216 | 164 | } | ||
217 | 165 | } | ||
218 | 166 | }} | ||
219 | 0 | 167 | ||
220 | === modified file 'charmworld/migrations/versions/tests/test_migrations.py' | |||
221 | --- charmworld/migrations/versions/tests/test_migrations.py 2014-04-22 20:36:12 +0000 | |||
222 | +++ charmworld/migrations/versions/tests/test_migrations.py 2014-05-20 14:13:54 +0000 | |||
223 | @@ -1,16 +1,22 @@ | |||
224 | 1 | # Copyright 2012, 2013 Canonical Ltd. This software is licensed under the | 1 | # Copyright 2012, 2013 Canonical Ltd. This software is licensed under the |
225 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). |
226 | 3 | 3 | ||
227 | 4 | from charmworld.models import ( | ||
228 | 5 | CharmSource, | ||
229 | 6 | ) | ||
230 | 7 | from charmworld.testing import ( | 4 | from charmworld.testing import ( |
231 | 8 | factory, | 5 | factory, |
232 | 9 | MongoTestBase, | 6 | MongoTestBase, |
233 | 10 | ) | 7 | ) |
234 | 11 | 8 | ||
235 | 12 | from charmworld.migrations import migrate | 9 | from charmworld.migrations import migrate |
237 | 13 | from charmworld.migrations.migrate import Versions | 10 | from charmworld.migrations.migrate import ( |
238 | 11 | DataStore, | ||
239 | 12 | Versions | ||
240 | 13 | ) | ||
241 | 14 | |||
242 | 15 | from charmworld.migrations.versions.tests.data import migration_027 as mdata | ||
243 | 16 | |||
244 | 17 | from charmworld.models import CharmSource | ||
245 | 18 | |||
246 | 19 | from pyelasticsearch.exceptions import ElasticHttpError | ||
247 | 14 | 20 | ||
248 | 15 | import logging | 21 | import logging |
249 | 16 | import mock | 22 | import mock |
250 | @@ -126,3 +132,72 @@ | |||
251 | 126 | def test_exodus_restrictions(self): | 132 | def test_exodus_restrictions(self): |
252 | 127 | """Ensure that deploy-from-scratch does not violate exodus rules.""" | 133 | """Ensure that deploy-from-scratch does not violate exodus rules.""" |
253 | 128 | self.versions._check_exodus(self.versions.list_pending(0)) | 134 | self.versions._check_exodus(self.versions.list_pending(0)) |
254 | 135 | |||
255 | 136 | |||
256 | 137 | class TestUseNgrams027(MigrationTestBase): | ||
257 | 138 | |||
258 | 139 | version = 27 | ||
259 | 140 | module_name = '027_use_ngrams.py' | ||
260 | 141 | |||
261 | 142 | def setUp(self): | ||
262 | 143 | super(TestUseNgrams027, self).setUp() | ||
263 | 144 | self.use_index_client() | ||
264 | 145 | self.data_store = DataStore(self.db) | ||
265 | 146 | self.charm_source = CharmSource(self.db, self.index_client) | ||
266 | 147 | |||
267 | 148 | def exists_in_index(self, id_): | ||
268 | 149 | return self.index_client.get(id_) is not None | ||
269 | 150 | |||
270 | 151 | def sync_index(self): | ||
271 | 152 | charm_ids = self.charm_source.sync_index() | ||
272 | 153 | while True: | ||
273 | 154 | try: | ||
274 | 155 | next(charm_ids) | ||
275 | 156 | except StopIteration: | ||
276 | 157 | break | ||
277 | 158 | |||
278 | 159 | def put_old_mapping(self): | ||
279 | 160 | # Delete the temp_index. | ||
280 | 161 | self.index_client.delete_index() | ||
281 | 162 | # Create an empty index. | ||
282 | 163 | self.index_client._client.create_index(self.index_client.index_name) | ||
283 | 164 | # Put up the old mappings. | ||
284 | 165 | for type_ in ('charm', 'bundle'): | ||
285 | 166 | self.index_client._client.put_mapping( | ||
286 | 167 | self.index_client.index_name, | ||
287 | 168 | type_, | ||
288 | 169 | getattr(mdata, 'old_{}_mapping'.format(type_))) | ||
289 | 170 | |||
290 | 171 | def test_index_updated(self): | ||
291 | 172 | """Ensure that running upgrade reindexes charms.""" | ||
292 | 173 | self.put_old_mapping() | ||
293 | 174 | self.versions.ensure_initialized(self.data_store, True) | ||
294 | 175 | ## Pretend upgrades through 26 are complete. | ||
295 | 176 | self.data_store.update_version(26) | ||
296 | 177 | charm = factory.get_charm_json() | ||
297 | 178 | self.db.charms.insert(charm) | ||
298 | 179 | self.assertFalse(self.exists_in_index(charm['_id'])) | ||
299 | 180 | self.sync_index() | ||
300 | 181 | self.assertTrue(self.exists_in_index(charm['_id'])) | ||
301 | 182 | self.versions.upgrade(self.data_store, self.index_client, True) | ||
302 | 183 | self.assertEquals(self.data_store.current_version, self.version) | ||
303 | 184 | self.assertTrue(self.exists_in_index(charm['_id'])) | ||
304 | 185 | |||
305 | 186 | def test_exception_raised(self): | ||
306 | 187 | """Putting new mapping without creating the filter and analyzer | ||
307 | 188 | raises the expected exception. | ||
308 | 189 | """ | ||
309 | 190 | self.versions.ensure_initialized(self.data_store, True) | ||
310 | 191 | ## Pretend upgrades through 26 are complete. | ||
311 | 192 | self.data_store.update_version(26) | ||
312 | 193 | self.put_old_mapping() | ||
313 | 194 | charm = factory.get_charm_json() | ||
314 | 195 | self.assertFalse(self.exists_in_index(charm['_id'])) | ||
315 | 196 | self.index_client.index_charm(charm) | ||
316 | 197 | self.assertTrue(self.exists_in_index(charm['_id'])) | ||
317 | 198 | with self.assertRaises(ElasticHttpError) as e: | ||
318 | 199 | self.index_client.put_mapping() | ||
319 | 200 | self.assertEquals(e.status_code, 400) | ||
320 | 201 | self.assertEquals(e.error, | ||
321 | 202 | u'MapperParsingException[Analyzer [n3_20grams] ' | ||
322 | 203 | u'not found for field [ngrams]]') | ||
323 | 129 | 204 | ||
324 | === modified file 'charmworld/search.py' | |||
325 | --- charmworld/search.py 2014-05-15 20:36:22 +0000 | |||
326 | +++ charmworld/search.py 2014-05-20 14:13:54 +0000 | |||
327 | @@ -180,43 +180,6 @@ | |||
328 | 180 | ] | 180 | ] |
329 | 181 | } | 181 | } |
330 | 182 | } | 182 | } |
331 | 183 | }, | ||
332 | 184 | "mappings": { | ||
333 | 185 | "charm": { | ||
334 | 186 | "properties": { | ||
335 | 187 | "name": { | ||
336 | 188 | "type": "multi_field", | ||
337 | 189 | "fields": { | ||
338 | 190 | "ngrams": { | ||
339 | 191 | "type": "string", | ||
340 | 192 | "analyzer": "n3_20grams" | ||
341 | 193 | }, | ||
342 | 194 | "name": { | ||
343 | 195 | "type": "string", | ||
344 | 196 | "index": "not_analyzed" | ||
345 | 197 | } | ||
346 | 198 | } | ||
347 | 199 | } | ||
348 | 200 | } | ||
349 | 201 | |||
350 | 202 | }, | ||
351 | 203 | "bundle": { | ||
352 | 204 | "properties": { | ||
353 | 205 | "name": { | ||
354 | 206 | "type": "multi_field", | ||
355 | 207 | "fields": { | ||
356 | 208 | "ngrams": { | ||
357 | 209 | "type": "string", | ||
358 | 210 | "analyzer": "n3_20grams" | ||
359 | 211 | }, | ||
360 | 212 | "name": { | ||
361 | 213 | "type": "string", | ||
362 | 214 | "index": "not_analyzed" | ||
363 | 215 | } | ||
364 | 216 | } | ||
365 | 217 | } | ||
366 | 218 | } | ||
367 | 219 | } | ||
368 | 220 | } | 183 | } |
369 | 221 | } | 184 | } |
370 | 222 | }) | 185 | }) |
Reed this branch looks great, modulo some stylistic issues.
The QA steps you provide are fantastic. The last two steps do make assumptions about what charms have been ingested locally.
Note this review is based on the version of your code as of 8:00am, not the branch you subsequently pushed.
I've marked the branch as approved, which means you are free to land after making (or discussing) the suggested changes. Had I wanted to see the changes made before you land I would've used 'needs fixing' but I don't, so I didn't.
Thanks for persevering and getting this in so we can do the deploy.