Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Sync GitHub internal repos during user perms sync#56677

Merged
pjlast merged 20 commits into
mainfrom
pjlast/list-github-internal-repos-for-user
Oct 4, 2023
Merged

Sync GitHub internal repos during user perms sync#56677
pjlast merged 20 commits into
mainfrom
pjlast/list-github-internal-repos-for-user

Conversation

@pjlast

@pjlast pjlast commented Sep 15, 2023

Copy link
Copy Markdown
Contributor

Closes #56648

Sourcegraph now explicitly fetches internal repositories for organizations a user belongs to. This used to be only done when groups caching is enabled, but it leads to a lot of confusion for customers.

This may increase the number of requests made by user permission syncs. If that is the case, groups caching should be enabled.

As part of this work I decided the refactor the GitHub user permissions fetching code, which was very contrived. It's a lot easier to reason about what should be happening now.

Test plan

VCR tests updated to account for extra API calls. Local validations done as well.

@cla-bot cla-bot Bot added the cla-signed label Sep 15, 2023
@pjlast pjlast marked this pull request as ready for review September 15, 2023 14:44
@sourcegraph-bot

sourcegraph-bot commented Sep 15, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 2193a1c...e3e5b05.

Notify File(s)
@eseliger internal/batches/sources/testdata/golden/GithubSource_GetFork_success_with_existing_changeset_and_existing_fork
internal/batches/sources/testdata/golden/GithubSource_GetFork_success_with_existing_changeset_and_new_fork
internal/batches/sources/testdata/golden/GithubSource_GetFork_success_with_new_changeset_and_existing_fork
internal/batches/sources/testdata/golden/GithubSource_GetFork_success_with_new_changeset_and_new_fork
internal/extsvc/github/common.go
internal/extsvc/github/testdata/golden/ListRepositoriesForSearch
internal/extsvc/github/testdata/golden/SearchRepos-Enterprise-narrow-query-enterprise
internal/extsvc/github/testdata/golden/TestGetRepository/cached-response/first_run
internal/extsvc/github/testdata/golden/TestGetRepository/cached-response/second_run
internal/extsvc/github/testdata/golden/TestGetRepository/forked_repo
internal/extsvc/github/testdata/golden/TestV3Client_Fork_success_sourcegraph-testing
internal/extsvc/github/testdata/golden/TestV3Client_Fork_success_user
internal/extsvc/github/v3.go
internal/extsvc/github/v3_test.go
internal/extsvc/github/v4.go
internal/repos/github.go
internal/repos/github_test.go
@unknwon cmd/frontend/internal/auth/githuboauth/session.go
cmd/repo-updater/internal/authz/BUILD.bazel
cmd/repo-updater/internal/authz/integration_test.go
cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubInternalRepositories.yaml
cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/groups-enabled.yaml
cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/no-groups.yaml
cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/groups-enabled.yaml
cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/no-groups.yaml
internal/authz/providers/github/BUILD.bazel
internal/authz/providers/github/BUILD.bazel
internal/authz/providers/github/authz.go
internal/authz/providers/github/authz.go
internal/authz/providers/github/client.go
internal/authz/providers/github/client.go
internal/authz/providers/github/github.go
internal/authz/providers/github/github.go
internal/authz/providers/github/github_test.go
internal/authz/providers/github/github_test.go
internal/authz/providers/github/mocks_test.go
internal/authz/providers/github/mocks_test.go

Comment thread internal/authz/providers/github/github.go Outdated
@pjlast pjlast requested a review from a team September 15, 2023 14:53

@unknwon unknwon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 (not an authoritative approval)

Comment thread CHANGELOG.md Outdated
Comment thread internal/authz/providers/github/github.go Outdated
Comment thread internal/authz/providers/github/github.go Outdated
Comment thread internal/authz/providers/github/github.go Outdated
Comment thread internal/authz/providers/github/github.go Outdated
@pjlast pjlast force-pushed the pjlast/list-github-internal-repos-for-user branch from f604eb0 to 51a9fc1 Compare September 18, 2023 10:10
@mrnugget mrnugget requested review from eseliger and ggilmore October 2, 2023 15:12

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By no means am I an expert in the authz stuff yet, but the code change makes logical sense to me .. hope that is enough 😆

Approving, but as we talked about today, would be great to also have the toggle to disable this changed behavior.

@pjlast pjlast force-pushed the pjlast/list-github-internal-repos-for-user branch from 0cffec1 to a3b7ae8 Compare October 4, 2023 10:01
@pjlast pjlast merged commit d271395 into main Oct 4, 2023
@pjlast pjlast deleted the pjlast/list-github-internal-repos-for-user branch October 4, 2023 14:43
sourcegraph-release-bot pushed a commit that referenced this pull request Oct 5, 2023
keegancsmith added a commit that referenced this pull request Oct 16, 2023
)

Add GitHub Auth config options to manage internal repos (#56677)

(cherry picked from commit d271395)

Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sourcegraph Doesn't Respect Repo Visibility Rules For GitHub Enterprise

5 participants