Skip to content
Snippets Groups Projects

Implement faster resolver for global campaign diff stat

Merged Administrator requested to merge es/faster-diffstat into main

Created by: eseliger

Before we had to load all the changesets, which including metadata can take quite a while for larger campaigns (quite some data to be sent). This approach asks the DB directly, while respecting repo permissions, and is close to instant, removing the need to lazy-load the diff stat on the campaigns page.

TODO: I can't figure out why the test fails, if you have an idea, please comment :) Otherwise I'll debug that next. Opening up for review anyways, since I assume the test fix won't change much.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: sourcegraph-bot

    Notifying subscribers in CODENOTIFY files for diff ab487c716a4c1392f6cad8e914b0819bea711b06...a370572f1b899a1bd3eba6bc2a402b2bf97824b7.

    Notify File(s)
    @LawnGnome enterprise/internal/campaigns/resolvers/campaign.go
    enterprise/internal/campaigns/store/campaigns.go
    enterprise/internal/campaigns/store/campaigns_test.go
    enterprise/internal/campaigns/testing/changeset.go
    enterprise/internal/campaigns/testing/repos.go
    internal/db/repos.go
    internal/db/repos_perm.go
    internal/db/repos_perm_test.go
    @asdine internal/db/repos.go
    internal/db/repos_perm.go
    internal/db/repos_perm_test.go
    @unknwon internal/db/repos_perm.go
    internal/db/repos_perm_test.go
  • Created by: eseliger

    @LawnGnome In case you ever need it: the repo needs to be private to enforce permissions on it. settings repo.Private = true fixes it.

  • Created by: LawnGnome

    @LawnGnome In case you ever need it: the repo needs to be private to enforce permissions on it. settings repo.Private = true fixes it.

    Thanks! Yeah, that's the kind of thing I just wouldn't have known without a deeper dive. Nice job. :thumbsup:

  • Created by: unknwon

    Sorry about trouble related to permissions @eseliger!

    I pushed a commit to fix the test, basically, the code host connection (external service) was not set to turn on authorization, so the visibility of repositories was disregarded.

  • Created by: codecov[bot]

    Codecov Report

    Merging #17529 (a370572) into main (ab487c7) will decrease coverage by 0.00%. The diff coverage is 75.00%.

    @@            Coverage Diff             @@
    ##             main   #17529      +/-   ##
    ==========================================
    - Coverage   50.32%   50.32%   -0.01%     
    ==========================================
      Files        1715     1715              
      Lines       85183    85183              
      Branches     7805     7805              
    ==========================================
    - Hits        42871    42865       -6     
    - Misses      38443    38447       +4     
    - Partials     3869     3871       +2     
    Flag Coverage Δ *Carryforward flag
    go 50.50% <75.00%> (-0.01%) :arrow_down:
    integration 30.68% <ø> (ø) Carriedforward from ab487c7
    typescript 49.87% <ø> (ø) Carriedforward from ab487c7
    unit 34.59% <ø> (ø) Carriedforward from ab487c7

    *This pull request uses carry forward flags. Click here to find out more.

    Impacted Files Coverage Δ
    internal/db/repos_perm.go 90.90% <ø> (ø)
    enterprise/internal/campaigns/store/campaigns.go 88.81% <66.66%> (-1.79%) :arrow_down:
    ...nterprise/internal/campaigns/resolvers/campaign.go 76.92% <100.00%> (-0.67%) :arrow_down:
    internal/db/repos.go 70.48% <100.00%> (ø)
    ...ternal/campaigns/resolvers/changeset_connection.go 69.23% <0.00%> (-3.85%) :arrow_down:
    ...terprise/internal/campaigns/resolvers/changeset.go 58.53% <0.00%> (-0.82%) :arrow_down:
Please register or sign in to reply
Loading