Merge ~racb/git-ubuntu:reimport-service into git-ubuntu:master

a5a60328982db821500f5157481ed1eb9c7627cb

+ parser.add_argument(
+ '-o',
+ '--lp-owner',
+ type=str,
+ help=(
+ "The Launchpad user to authenticate against for the imports, as "
+ "passed to 'git ubuntu import -l'"

I think the help text here is swapped with the text for --lp-user.

Also, just for consistency list the parser.add_argument for --lp-user first, and --lp-owner second, so the order is consistent with the ordering of lp_user, lp_owner in the rest of the code. (I'll bet the reversed order is what led to the above typo.)

The distinction between --lp-user and --lp-owner was not immediately clear to me, and the docs didn't clarify it in my head. Looking at importer.py I see that lp_user is what's used for authenticated communications with launchpad, whereas lp_owner is the owner for the git repository that will be created or updated. And, I can see the confusion comes from the importer.py parameter descriptions, your branch simply inherits the confusion.

Anyway, I think this might be clearer help description for --lp-owner:

+ "The username that will own the git repository being imported, as "
+ "passed to 'git ubuntu import -o'"

c754c56a1aa00315d9a8d61df6b2716b5b6eb66a

The 'tuple(str, bool)' mentioned as the standard type being used for a "request" makes me wonder if this might be worth creating a little class for, so functions can pass better defined objects. Then instead of a bool for import vs. reimport you could use an enumeration like you did for PatchState (c.f. 369f645c); you're casting the bool to 0 or 1 in several places already, so it feels like it wants to be an enum. From the FIXME comment I wonder if being able to track "pending" or other states might be of use some day, so an enum may be more extensible than a bool.

Nice work cleaning up so much codedocs, and the test cases look good as well; I see what you mean about some of it being difficult to write tests for, but there's more test cases than I thought there'd be.

typo: requset -> request

The rest looks good, easy to follow and makes sense. I didn't have time to run the testsuite but will do so on the followup review.

PASSED: Continuous integration, rev:a5a60328982db821500f5157481ed1eb9c7627cb
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/516/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/516//rebuild

Thank you for the review!

> I think the help text here is swapped with the text for --lp-user.

Fixed and squashed in.

> typo: requset -> request

Fixed and squashed in.

> Also, just for consistency list the parser.add_argument for --lp-user first, and --lp-owner second, so the order is consistent with the ordering of lp_user, lp_owner in the rest of the code. (I'll bet the reversed order is what led to the above typo.)

Fixed and squashed in. Thanks for spotting that. The reversed order is indeed what led to the error. I was inconsistent initially, tried to make it consistent and missed that instance!

> Anyway, I think this might be clearer help description for --lp-owner:

Fixed and squashed in, with one minor tweak - I wanted to make it clear that the owner could be a team.

> The 'tuple(str, bool)' mentioned as the standard type being used for a "request" makes me wonder if this might be worth creating a little class for

Done. This was quite extensive so I've done it as a separate commit.

Here's the range diff: https://paste.ubuntu.com/p/qDX65PkS99/

PASSED: Continuous integration, rev:d849b11efa648400c2db38c9457578f6682eadd7
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/517/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/517//rebuild

Very nice! And thanks for the range diff, I always forget to do that but it's quite helpful.

I did spot a couple typos, feel free to fix up when landing, no re-review needed:

* "These numbers are hardcoded becuase they must match the what we use"
  - sp. "becuase"
  - del 'the'