launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

A relatively clean place way to split this would have been s/branch/target/ in an earlier branch. But it's easy enough to scroll over.

Diff comments:

> 
> === modified file 'lib/lp/code/mail/codeimport.py'
> --- lib/lp/code/mail/codeimport.py	2016-10-03 17:03:12 +0000
> +++ lib/lp/code/mail/codeimport.py	2016-10-03 17:03:17 +0000
> @@ -61,7 +61,7 @@
>          user.displayname, user.preferredemail.email)
>  
>      vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
> -    headers = {'X-Launchpad-Branch': code_import.branch.unique_name,
> +    headers = {'X-Launchpad-Branch': code_import.target.unique_name,

This seems somewhat unfortunate, but I'm not sure what would be better.

>                 'X-Launchpad-Message-Rationale':
>                     'Operator @%s' % vcs_imports.name,
>                 'X-Launchpad-Message-For': vcs_imports.name,
> 
> === modified file 'lib/lp/code/model/codeimport.py'
> --- lib/lp/code/model/codeimport.py	2016-09-30 13:27:27 +0000
> +++ lib/lp/code/model/codeimport.py	2016-10-03 17:03:17 +0000
> @@ -175,12 +206,14 @@
>          new_whiteboard = None
>          if 'whiteboard' in data:
>              whiteboard = data.pop('whiteboard')
> -            if whiteboard != self.branch.whiteboard:
> -                if whiteboard is None:
> -                    new_whiteboard = ''
> -                else:
> -                    new_whiteboard = whiteboard
> -                self.branch.whiteboard = whiteboard
> +            # XXX cjwatson 2016-10-03: Do we need something similar for Git?
> +            if self.branch is not None:
> +                if whiteboard != self.branch.whiteboard:
> +                    if whiteboard is None:
> +                        new_whiteboard = ''
> +                    else:
> +                        new_whiteboard = whiteboard
> +                    self.branch.whiteboard = whiteboard

lolwut. Branch whiteboards haven't been a thing for years, and I assumed CodeImport had its own rather than just being the sole remaining user of Branch.whiteboard.

>          token = event_set.beginModify(self)
>          for name, value in data.items():
>              setattr(self, name, value)
> 
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/code/model/codeimportjob.py	2016-10-03 17:03:17 +0000
> @@ -311,7 +311,8 @@
>              naked_import.date_last_successful = result.date_created
>          # If the status was successful and revisions were imported, arrange
>          # for the branch to be mirrored.
> -        if status == CodeImportResultStatus.SUCCESS:
> +        if (status == CodeImportResultStatus.SUCCESS and
> +                code_import.branch is not None):
>              code_import.branch.requestMirror()

This is actually slightly interesting, because for Git the LP side of things will be updated before the import job completes. I don't think it's a problem unless we run into some lock contention that I can't immediately think of.

>          getUtility(ICodeImportEventSet).newFinish(
>              code_import, machine)


-- 
https://code.launchpad.net/~cjwatson/launchpad/codeimport-git-model/+merge/307466
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.