launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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.