[Merge] lp:~malizor/update-manager/ppa-changelogs into lp:update-manager

[Brian Murray]

Thanks for working on this, I appreciate it a lot.  Generally, this looks good to me, but I have some comments which you'll find in line.  You might also want to add a changelog entry to debian/changelog.

Diff comments:

> === modified file 'UpdateManager/Core/MyCache.py'
> --- UpdateManager/Core/MyCache.py	2014-06-26 07:03:08 +0000
> +++ UpdateManager/Core/MyCache.py	2016-06-10 23:11:06 +0000
> @@ -268,6 +274,52 @@
>              alllines = alllines + line
>          return alllines
>  
> +    def _extract_ppa_changelog_uri(self, name):
> +        """Return the changelog URI from the Launchpad API
> +
> +        Return None in case of an error.
> +        """
> +        if Launchpad is None:
> +            logging.warning("Please install 'python3-launchpadlib' to enable "
> +                            "changelog retrieval for PPAs.")
> +            return
> +        cdt = self[name].candidate
> +        for uri in cdt.uris:
> +            match = re.search('http.*/(.*)/(.*)/ubuntu/.*', uri)
> +            if match is not None:
> +                user, ppa = match.group(1), match.group(2)
> +                break
> +        else:
> +            logging.error('Unable to extract the changelog from the PPA')

This same message is logged in three different situations which could make debugging difficult.  I think it would be better to use a different message for each specific situation.  Additionally, it'd be nice to end each sentence with a period.

> +            return
> +
> +        # Login on launchpad if we are not already
> +        if self.launchpad is None:
> +            try:
> +                self.launchpad = Launchpad.login_anonymously('update-manager',
> +                                                             'production',
> +                                                             version='devel')
> +            except:
> +                logging.exception("Unable to connect to Launchpad to retrieve "
> +                                  "the changelog")
> +                return
> +
> +        archive = self.launchpad.archives.getByReference(
> +            reference='~%s/ubuntu/%s' % (user, ppa)
> +        )
> +        if archive is None:
> +            logging.error('Unable to extract the changelog from the PPA')

This is the 2nd message with the same string.

> +            return
> +
> +        spph = archive.getPublishedSources(source_name=cdt.source_name,
> +                                           exact_match=True,
> +                                           version=cdt.version)
> +        if not spph:
> +            logging.error('Unable to extract the changelog from the PPA')

And the 3rd message with the same string.

> +            return
> +
> +        return spph[0].changelogUrl()

It'd probably be a good idea to guard against a key error here.

> +
>      def _guess_third_party_changelogs_uri_by_source(self, name):
>          pkg = self[name]
>          deb_uri = pkg.candidate.uri


-- 
https://code.launchpad.net/~malizor/update-manager/ppa-changelogs/+merge/297119
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~malizor/update-manager/ppa-changelogs into lp:update-manager.