launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/interfaces/gitapi.py b/lib/lp/code/interfaces/gitapi.py
> index c7db491..7a01517 100644
> --- a/lib/lp/code/interfaces/gitapi.py
> +++ b/lib/lp/code/interfaces/gitapi.py
> @@ -79,3 +79,10 @@ class IGitAPI(Interface):
>  
>          :returns: A list of rules for the user in the specified repository
>          """
> +
> +    def getMPurlRPC(translated_paths, branch, auth_params):

"RPC" is redundant here.  Can you make this "getMergeProposalURL" instead?

> +        """Return the URL for a Merge Proposal for a `branch` in a `repository`..
> +
> +        :returns: The URL for a Merge Proposal for the branch in the
> +            specified repository
> +        """
> \ No newline at end of file
> diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
> index cbf9205..61dbbe2 100644
> --- a/lib/lp/code/xmlrpc/git.py
> +++ b/lib/lp/code/xmlrpc/git.py
> @@ -24,6 +24,7 @@ from zope.error.interfaces import IErrorReportingUtility
>  from zope.interface import implementer
>  from zope.security.interfaces import Unauthorized
>  from zope.security.proxy import removeSecurityProxy
> +from lp.services.config import config

This import is in the wrong section (third-party imports rather than Launchpad imports).  Use utilities/format-new-and-modified-imports.

>  
>  from lp.app.errors import NameLookupFailed
>  from lp.app.validators import LaunchpadValidationError
> @@ -72,7 +74,7 @@ from lp.services.macaroons.interfaces import (
>      IMacaroonIssuer,
>      NO_USER,
>      )
> -from lp.services.webapp import LaunchpadXMLRPCView
> +from lp.services.webapp import LaunchpadXMLRPCView, canonical_url

Misformatted, but utilities/format-new-and-modified-imports will take care of that too.

>  from lp.services.webapp.authorization import check_permission
>  from lp.services.webapp.errorlog import ScriptRequest
>  from lp.xmlrpc import faults
> @@ -426,6 +428,25 @@ class GitAPI(LaunchpadXMLRPCView):
>              removeSecurityProxy(repository))
>          logger.info("notify succeeded")
>  
> +    def getMPurlRPC(self, translated_path, branch):
> +        """See `IGitAPI`."""
> +        logger = self._getLogger()
> +        logger.info("Request received: getMPurlRPC('%s')", translated_path)
> +        repository = getUtility(IGitLookup).getByHostingPath(translated_path)
> +        if repository is None:
> +            fault = faults.NotFound(
> +                "No repository found for '%s'." % translated_path)
> +            logger.error("getMPurlRPC failed: %r", fault)
> +            return fault
> +
> +        base_url = canonical_url(repository, rootsite='code')
> +        if 'refs/heads/'+branch != repository.default_branch:
> +            mp_url = (base_url+'/+ref/'+branch+'/+register-merge')

Put spaces around binary operators like +, please.  And when assembling strings, it's usually clearer to use some kind of formatting operation (% or .format) rather than a pile-up of + operators.

I think there should be a comment here explaining that you can't assemble the URL in the normal way because the ref may not exist yet.

The branch name may contain characters that require quoting in URLs: for example, "%" is a valid character in branch names.  You'll need to take that into account, probably just by using six.moves.urllib.parse.quote.

> +            logger.info("getMPurlRPC succeeded in forming MP URL : '%s'" % mp_url)

There shouldn't normally be a space before a colon.

I'd probably phrase this as just:

  logger.info("getMergeProposalURL succeeded: %s" % mp_url)

> +            return mp_url
> +
> +        logger.info("getMPurlRPC succeeded")

The log message is rather confusing and could do with being more explicit.

The interface docstring doesn't say anything about returning None if the branch name is the default branch.

I think it would probably make more sense to do the default-branch check on the turnip side rather than here, by looking up the target of the symbolic ref HEAD; doing it only there would make this interface a lot clearer.

> +
>      @return_fault
>      def _authenticateWithPassword(self, username, password):
>          """Authenticate a user by username and password.


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/377044
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:add-mp-url-to-git-push into launchpad:master.