Code review comment for lp:~reedobrien/charmworld/es-migration

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/migrations/versions/tests/__init__.py 1970-01-01
> 00:00:00 +0000
> > +++ charmworld/migrations/versions/tests/__init__.py 2014-05-20
> 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/migrations/versions/tests/test_migrations.py'
> > --- charmworld/migrations/versions/tests/test_migrations.py 2014-04-22
> 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.exceptions import (
> > + ElasticHttpError,
> > + ElasticHttpNotFoundError,
> > +)
> >
> > import logging
> > import mock
> > @@ -126,3 +138,79 @@
> > def test_exodus_restrictions(self):
> > """Ensure that deploy-from-scratch does not violate exodus
> rules."""
> > self.versions._check_exodus(self.versions.list_pending(0))
> > +
> > +
> > +class TestUseNgrams027(MigrationTestBase):
> > +
> > + version = 27
> > + module_name = '027_use_ngrams.py'
> > +
> > + def setUp(self):
> > + super(TestUseNgrams027, self).setUp()
> > + self.use_index_client()
> > + self.ds = DataStore(self.db)
> > + self.cs = CharmSource(self.db, self.index_client)
>
> 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_in_index(self, id_, kind=CHARM):
> > + client = self.index_client
> > + if kind == CHARM:
> > + return client.get(id_) is not None
> > + try:
> > + result = client._client.get(client.index_name, kind, id_)
> > + except ElasticHttpNotFoundError:
> > + return False
> > + return result['exists']
> > +
> > + def sync_index(self):
>
> I found this simpler function works:
>
> def exists_in_index(self, id_, kind=CHARM):
> client = self.index_client
> return client.get(id_) is not None
>

got it down to:

    def exists_in_index(self, id_):
        return self.index_client.get(id_) is not None

We are only inserting one charm in the test. Adding a bunch of if
BUNDLE/CHARM
 variation seems noisy. If inserting a charm comes up as a bundle there is
a problem
elsewhere and the tests for that should be with that code.

> > + charm_ids = self.cs.sync_index()
> > + while True:
> > + try:
> > + next(charm_ids)
> > + except StopIteration:
> > + break
> > +
> > + def put_old_mapping(self):
> > + # Delete the temp_index.
> > + self.index_client.delete_index()
> > + # Create an empty index.
> > +
> self.index_client._client.create_index(self.index_client.index_name)
> > + # Put up the old mappings.
> > + for typ in ("charm", "bundle"):
> > + self.index_client._client.put_mapping(
>
> I'd suggest 'type_' or 'kind' instead of the non-word 'typ'.
>

--> type_ because it is an elasticsearch type not an elasticsearch kind.

> > + self.versions.ensure_initialized(self.ds, True)
> > + ## Pretend upgrades through 26 are complete.
> > + self.ds.update_version(26)
> > + charm = factory.get_charm_json()
> > + self.db.charms.insert(charm)
> > + self.assertFalse(self.exists_in_index(charm['_id']))
> > + self.sync_index()
> > + self.assertTrue(self.exists_in_index(charm['_id']))
> > + self.versions.upgrade(self.ds, self.index_client, True)
> > + self.assertEquals(self.ds.current_version, self.version)
> > + self.assertTrue(self.exists_in_index(charm['_id']))
> > +
> > + def test_exception_raised(self):
> > + """Putting new mapping without creating the filter and analyzer
> > + raises the expected exception."""
> > + self.versions.ensure_initialized(self.ds, True)
>
> Multi-line docstrings should have the closing """ on a newline. (Damn
> PEP-8)
>

My linter pep8flaker doesn't whine about that, possibly because that is
PEP-257...

Someone should just write pyfmt, dammit.

Either way --> \n'ed. Also "->' in here.

> > + ## Hopefully we aren't targeting pre py2.7
> > + with self.assertRaises(ElasticHttpError) as e:
>
> We are not. You can delete the comment.
>

Struck.

>
>
> --
>
> https://code.launchpad.net/~reedobrien/charmworld/es-migration/+merge/220144
> You are the owner of lp:~reedobrien/charmworld/es-migration.
>

--
Reed O'Brien
✉ <email address hidden>
✆ 415-562-6797