Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options to configure internal repo handling for GitLab #57858

Merged
merged 17 commits into from Oct 31, 2023

Conversation

pjlast
Copy link
Contributor

@pjlast pjlast commented Oct 24, 2023

Closes #57804

This PR adds an option to GitLab code host connections called markInternalReposAsPublic, which sets the visibility of all internal repos added by this code host connection to true, and an option to the GitLab auth provider called syncInternalRepoPermissions. syncInternalRepoPermissions are true by default, and can be set to false to optimise systems where markInternalReposAsPublic is set to true.

Test plan

Unit tests updated. Set up and tested locally as well.

@cla-bot cla-bot bot added the cla-signed label Oct 24, 2023
@github-actions github-actions bot added the team/source Tickets under the purview of Source - the one Source to graph it all label Oct 24, 2023
@pjlast pjlast requested a review from a team October 25, 2023 12:07
@pjlast pjlast marked this pull request as ready for review October 25, 2023 12:07
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Oct 25, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff 17e8f2d...114d762.

Notify File(s)
@eseliger internal/repos/gitlab.go
internal/repos/gitlab_test.go
@unknwon cmd/repo-updater/internal/authz/integration_test.go
internal/authz/providers/authz_test.go
internal/authz/providers/authz_test.go
internal/authz/providers/gitlab/authz.go
internal/authz/providers/gitlab/authz.go
internal/authz/providers/gitlab/oauth.go
internal/authz/providers/gitlab/oauth.go
internal/authz/providers/gitlab/oauth_test.go
internal/authz/providers/gitlab/oauth_test.go
internal/authz/providers/gitlab/sudo.go
internal/authz/providers/gitlab/sudo.go
internal/authz/providers/gitlab/sudo_test.go
internal/authz/providers/gitlab/sudo_test.go

}
```

If you would like internal repositories to remain private, but you're experiencing issues where user permission syncs aren't granting access to internal repositories, you can add the following field instead:
Copy link
Member

Choose a reason for hiding this comment

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

when would I experience these issues and how do I notice that I do?

Comment on lines +241 to +247
repoVisibility := []string{"private"}
if listInternalRepos {
repoVisibility = append(repoVisibility, "internal")
}

// This method is meant to return only private or internal projects
for _, visibility := range []string{"private", "internal"} {
for _, visibility := range repoVisibility {
Copy link
Member

Choose a reason for hiding this comment

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

This changes the default here, correct? Would this cause any trouble to existing instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm still thinking about this a bit. I'm flip-flopping a bit between what would be the better option.

I don't mind changing the default behaviour too much, as long as we mention it in the changelog. And the behavioural change won't be particularly destructive.

I'd just like to settle on a default behaviour that would work for most cases. Which might be: private internal repos with syncing enabled 🤔 will let it stew a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably just condense the settings to the single markInternalReposAsPublic one? Since if they're public, you don't need permission syncs 🤔

provider: provider,
client: client,
logger: logger,
markInternalReposAsPublic: (c.Authorization != nil) && c.Authorization.MarkInternalReposAsPublic,
Copy link
Member

Choose a reason for hiding this comment

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

hmm do we need that c.Authorization check here?
Is it to cover the case where someone sets it to false, but auth is off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Authorization can be nil here if it is completely unset, so accessing .MarkInternalReposAsPublic would throw a nil pointer exception

internal/repos/gitlab.go Outdated Show resolved Hide resolved
@pjlast pjlast force-pushed the pjlast/57804-default-permissions-gitlab-internal-repos branch from 4b0c373 to 66f946d Compare October 31, 2023 11:18
@pjlast pjlast merged commit 2ea61e5 into main Oct 31, 2023
10 checks passed
@pjlast pjlast deleted the pjlast/57804-default-permissions-gitlab-internal-repos branch October 31, 2023 12:37
sourcegraph-release-guild-bot pushed a commit that referenced this pull request Oct 31, 2023
camdencheek pushed a commit that referenced this pull request Nov 1, 2023
…tLab (#58012)

Add options to configure internal repo handling for GitLab (#57858)

(cherry picked from commit 2ea61e5)

Co-authored-by: Petri-Johan Last <petri.last@sourcegraph.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 5.2 cla-signed team/source Tickets under the purview of Source - the one Source to graph it all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to set default permission value for GitLab internal repos (like we do for GitHub)
3 participants