git-ubuntu

Merge ~racb/git-ubuntu:set-as-default-repository into git-ubuntu:main

  1. Git
  2. lp:~racb/git-ubuntu
  3. set-as-default-repository
  4. Merge into main
Proposed by Robie Basak
Status: Merged
Merged at revision: 32ed90abfbf6475e8d4e582b3e5e8e50a1f9dae5
Proposed branch: ~racb/git-ubuntu:set-as-default-repository
Merge into: git-ubuntu:main
Diff against target: 200 lines (+54/-2)
2 files modified
gitubuntu/importer.py (+22/-1)
gitubuntu/importer_service_worker.py (+32/-1)
Reviewer Review Type Date Requested Status
Andreas Hasenack Approve
Server Team CI bot continuous-integration Approve
Canonical Server Reporter Pending
Review via email: mp+440324@code.staging.launchpad.net

Commit message

Make Jenkins happy

Description of the change

You might notice that I haven't added any tests.

Fundamentally we're adding a Launchpad API call, glued to a new CLI argument, and that's it. I don't think that testing that glue is valuable - it's just a single parameter being passed through. We could mock LP and check that the API call is happening, but again it's a single line. What is most likely to go wrong is some interaction between us and Launchpad's API, but just testing that we're calling the API in the way we're calling the API wouldn't find that.

Instead I've checked manually in production that "git ubuntu import --push --set-as-default-repository ..." works against a real repository that wasn't previously set, and locally that "git ubuntu importer-service-worker --dry-run ..." reports that it would call "git ubuntu import" with "--set-as-default-repository" as expected, and that "git ubuntu importer-service-worker --dry-run --no-set-as-default-repository" does omit "--set-as-default-repository" in what it reports it would call.

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:32ed90abfbf6475e8d4e582b3e5e8e50a1f9dae5
https://jenkins.canonical.com/server-team/job/git-ubuntu-ci/19/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Taking a look at this now.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

+1, just a quick comment inline I thought about.

review: Approve
Revision history for this message
Robie Basak (racb) wrote :

On Thu, Apr 06, 2023 at 08:47:05PM -0000, Andreas Hasenack wrote:
> I presume you set things up for git bot account already to get this to
> work, and that launchpad has deployed their side of things into
> production already.

Yes - and I tested this.

> Should we fail hard here if this new api call fails, though? If it
> fails, will the import run be marked as failed, and be retried later?
> At which stage I presume the actual import will be a no-op, and it
> will just try again to set the default repository.

I think it should hard fail, like most other Launchpad API calls. I
deliberately set --set-as-default-repository to default to not set for
"git ubuntu import", so we can easily run it by hand without this
feature if required. And although "git ubuntu importer-service-worker"
does default to using it, there's also a --no-set-as-default-repository
switch to turn it off, which we could fairly easily do in production by
adjusting the worker systemd service. So I think there are sufficient
sysadmin knobs in case it just stops working for some reason.

If it fails then the entire import fails, and fall back on the existing
(fairly dumb) "retry every day for a few days" behaviour. But I think
this is unlikely - it is more likely to fail for everything than just
for the odd import. I tested for that, and we have the knobs to adjust
behaviour as above, without changing code, if required.

> This also means we are always, on every import, calling
> setDefaultRepository, which I also presume is not a big deal for LP.

Right. We already set HEAD on every import by Launchpad API call, so I
don't think this is a significant additional burden.

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches