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