Review: Approve
Diff comments:
>
> === modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
> --- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2014-10-29
> 06:04:09 +0000
> +++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py 2016-07-13
> 20:42:37 +0000
> @@ -274,6 +275,22 @@
> *extra_expr)
> return new_ppa_tokens
>
> + def getDeactivatedNamedTokens(self, since=None):
> + """Return result set of named tokens deactivated since given time."""
> + now = datetime.now(pytz.UTC)
> +
> + store = IStore(ArchiveAuthToken)
> + extra_expr = []
> + if since:
> + extra_expr = [ArchiveAuthToken.date_deactivated >= since]
> + tokens = store.find(
> + ArchiveAuthToken,
> + ArchiveAuthToken.name != None,
> + ArchiveAuthToken.date_deactivated != None,
> + ArchiveAuthToken.date_deactivated <= now,
"NULL <= now" is always NULL, so the "ArchiveAuthToken.date_deactivated !=
None" clause is unnecessary. (I appreciate you probably got this from
expireSubscriptions.)
That said, I'm not sure I totally understand why the "<= now" check is there,
but this code has been through some subtle work in the past to avoid strange
race conditions, so it might be best to leave that in place rather than rocking
the boat.
> + *extra_expr)
> + return tokens
> +
> def getNewPrivatePPAs(self, since=None):
> """Return the recently created private PPAs."""
> store = IStore(Archive)
>
> === modified file
> 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
> --- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py
> 2016-01-26 15:47:37 +0000
> +++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py
> 2016-07-13 20:42:37 +0000
> @@ -375,6 +382,32 @@
> os.remove(htaccess)
> os.remove(htpasswd)
>
> + def testBasicOperation_with_named_tokens(self):
> + """Invoke the actual script and make sure it generates some files."""
> + token1 = self.ppa.newNamedAuthToken(u"tokenname1", as_dict=False)
> + token2 = self.ppa.newNamedAuthToken(u"tokenname2", as_dict=False)
> + token3 = self.ppa.newNamedAuthToken(u"tokenname3", as_dict=False)
> + token3.deactivate()
> +
> + # Call the script and check that we have a .htaccess and a .htpasswd.
> + htaccess, htpasswd = self.ensureNoFiles()
> + script = self.getScript()
> + script.main()
> + self.assertThat([htaccess, htpasswd], AllMatch(FileExists()))
> + self.assertIn('+' + token1.name, open(htpasswd).read())
I'd prefer the context manager form in new code, to ensure that the file handle
is closed.
> + self.assertIn('+' + token2.name, open(htpasswd).read())
> + self.assertNotIn('+' + token3.name, open(htpasswd).read())
> +
> + # Deactivate a named token and verify it is removed from .htpasswd.
> + token2.deactivate()
> + script.main()
> + self.assertThat([htaccess, htpasswd], AllMatch(FileExists()))
> + self.assertIn('+' + token1.name, open(htpasswd).read())
> + self.assertNotIn('+' + token2.name, open(htpasswd).read())
> + self.assertNotIn('+' + token3.name, open(htpasswd).read())
> + os.remove(htaccess)
> + os.remove(htpasswd)
> +
> def _setupOptionsData(self):
> """Setup test data for option testing."""
> subs, tokens = self.setupDummyTokens()
>
> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2016-07-13 20:42:37 +0000
> +++ lib/lp/soyuz/model/archive.py 2016-07-13 20:42:37 +0000
> @@ -1952,7 +1952,7 @@
> IStore(ArchiveAuthToken).add(archive_auth_token)
> return archive_auth_token
>
> - def newNamedAuthToken(self, name, token=None):
> + def newNamedAuthToken(self, name, token=None, as_dict=True):
I'd do this a bit differently. My suggestion would be to use as_dict=False
here, add as_dict=False to the declaration in lp.soyuz.interfaces.archive as
well, and add the @call_with(as_dict=True) decorator to the interface,
adjusting the docstring to match. That way, you can have newNamedAuthToken
return the token internally by default while still returning the dict over the
webservice; most of your tests will be a bit less verbose as a result; and in
the few cases where you want the dict internally you can still pass
as_dict=True.
> """See `IArchive`."""
>
> if not getFeatureFlag(NAMED_AUTH_TOKEN_FEATURE_FLAG):
--
https://code.launchpad.net/~maxiberta/launchpad/named-auth-tokens-htaccess/+merge/300002
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
