On Wed, Mar 31, 2010 at 2:00 AM, Guilherme Salgado
<email address hidden> wrote:

> Remove the auth store, together with c-i-p.
>
> Had to grant some extra DB permissions on security.cfg because some
> things that used to be done using the launchpad_auth user (the one
> always used for the auth store, regardless of what was given to
> initZopeless) are now done using whichever user is used by the
> script/test.

Yay. This is more than I expected.



> === modified file 'lib/canonical/launchpad/database/tests/test_oauth.py'
> --- lib/canonical/launchpad/database/tests/test_oauth.py        2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/database/tests/test_oauth.py        2010-03-30 19:00:44 +0000
> @@ -22,7 +22,7 @@
>     """Base tests for the OAuth database classes."""
>     layer = DatabaseFunctionalLayer
>
> -    def test__get_store_should_return_the_auth_master_store(self):
> +    def test__get_store_should_return_the_main_master_store(self):
>         """We want all OAuth classes to use the master store.
>         Otherwise, the OAuth exchanges will fail because the authorize
>         screen won't probably find the new request token on the slave store.

Does this test serve any purpose any more? Maybe remove it entirely.


> === modified file 'lib/canonical/launchpad/webapp/adapter.py'
>         try:
> +            # XXX: Salgado, 2010-03-22: We can get rid of the realm now that
> +            # the auth DB is gone, but I'm not doing it in this branch to keep
> +            # the diff simple.
>             config_section, realm, flavor = self._uri.database.split('-')
>         except ValueError:
>             raise AssertionError(
>                 'Connection uri %s does not match section-realm-flavor format'
>                 % repr(self._uri.database))

I don't think this XXX is warranted. We may well have more realms in
the future, for example if we move translations or codehosting to
their own database for scalability reasons.

> === modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter_permissions.txt'
> -
> -A MASTER_FLAVOR Store does not allow writes to tables outside of that
> -Store's replication set.
> -
> -    >>> t = transaction.begin()
> -    >>> person = main_master.find(Person, name='no-priv').one()
> -    >>> account = person.account
> -    >>> account.displayname = "Ben Dover"
> -    >>> main_master.flush()
> -    Traceback (most recent call last):
> -    ...
> -    ProgrammingError: permission denied for relation account
> -    >>> transaction.abort()
> -
> -    >>> t = transaction.begin()
> -    >>> auth_master = getUtility(IStoreSelector).get(AUTH_STORE, MASTER_FLAVOR)
> -    >>> person = auth_master.find(Person, name='no-priv').one()
> -    >>> account = person.account
> -    >>> account.displayname = "Ben Dover"
> -    >>> auth_master.flush()
> -    >>> transaction.abort()

But this crap can certainly go. If we split the database in the
future, there will be no crossover. What I've learned from the
authdb/lpmain split is that the split needs to be clean, or we end up
with confusing code (is my email address from the authdb Store or the
lpmain Store) and a complicated and tricky to maintain replication
setup. The underlying approach mapping the database class to a Storm
store is a suitable approach to the sort of sharding that Launchpad
will need one day.



> +    # XXX: 2010-03-24, Guilherme Salgado: The 'name' argument will always be
> +    # MAIN_STORE here, so it can be removed.  Not doing it now as this branch
> +    # will be CPed into production.
>     def getStore(name, flavor):
>         """Retrieve a Store.

I think this XXX can go do. I'd rather not re implement this next year :)


> @@ -843,6 +843,9 @@
>     def get_current():
>         """Return the currently installed `IDatabasePolicy`."""
>
> +    # XXX: 2010-03-24, Guilherme Salgado: The 'name' argument will always be
> +    # MAIN_STORE here, so it can be removed.  Not doing it now as this branch
> +    # will be CPed into production.
>     def get(name, flavor):
>         """Retrieve a Storm Store.

And here.


> === modified file 'lib/canonical/launchpad/webapp/tests/test_dbpolicy.py'
> --- lib/canonical/launchpad/webapp/tests/test_dbpolicy.py       2010-02-24 23:18:40 +0000
> +++ lib/canonical/launchpad/webapp/tests/test_dbpolicy.py       2010-03-30 19:00:44 +0000
> @@ -25,7 +25,7 @@
>     ReadOnlyLaunchpadDatabasePolicy, SlaveDatabasePolicy,
>     SlaveOnlyDatabasePolicy)
>  from canonical.launchpad.webapp.interfaces import (
> -    ALL_STORES, AUTH_STORE, DEFAULT_FLAVOR, DisallowedStore, IDatabasePolicy,
> +    DEFAULT_FLAVOR, DisallowedStore, IDatabasePolicy,
>     IStoreSelector, MAIN_STORE, MASTER_FLAVOR, ReadOnlyModeDisallowedStore,
>     SLAVE_FLAVOR)

I'm tempted to say revert the changes in this file and put back
ALL_STORES, which would be a list of length one. It will also reduce a
lot off diff lines in this CP scary branch :)



>     def test_dbusers(self):
>         store_selector = getUtility(IStoreSelector)
>         main_store = store_selector.get(MAIN_STORE, DEFAULT_FLAVOR)
>         self.failUnlessEqual(self.getDBUser(main_store), 'launchpad_main')
>
> -        auth_store = store_selector.get(AUTH_STORE, DEFAULT_FLAVOR)
> -        self.failUnlessEqual(self.getDBUser(auth_store), 'launchpad_auth')

Well... not all the changes. Explicit references to AUTH_STORE need to
go, but the ALL_STORES loops seem fine.



Surprisingly few comments for a large diff. Thanks :)