Add feature flags to perms syncer context#56492
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff edd2096...c879393.
|
…/add-feature-flags-to-context-gitlab
mrnugget
left a comment
There was a problem hiding this comment.
Thanks! I think the test could be a bit smaller & cleaner
|
|
||
| ### Fixed | ||
|
|
||
| - Fixed an issue where the "gitLabProjectVisibilityExperimental" feature flag would not be respected by the permissions syncer [#56492](https://github.com/sourcegraph/sourcegraph/pull/56492) |
There was a problem hiding this comment.
Can you also add what the non-respecting-of-feature-flag lead to and how the behaviour is different now?
| CreatedAt: timeutil.Now(), | ||
| Config: extsvc.NewUnencryptedConfig(`{"url": "https://gitlab.sgdev.org", "authorization": {"identityProvider": {"type": "oauth"}}, "token": "abc", "projectQuery": [ "projects?membership=true&archived=no" ],}`), |
There was a problem hiding this comment.
Do you need all of this data? At least CreatedAt will be set by the Upsert further down, no?
There was a problem hiding this comment.
The rest of the data is required for the integration test. I removed the CreatedAt, and then I factored out the test setup and made an assert function like you suggested. No reason to re-do the entire DB.
| // https://gitlab.sgdev.org, then check if permissions are correctly granted for the test | ||
| // user "sourcegraph-vcr". | ||
| t.Run("user-centric", func(t *testing.T) { | ||
| t.Run("featureflag-enabled", func(t *testing.T) { |
There was a problem hiding this comment.
It's not super clear to me what the difference is between these two tests. Is it just the one FeatureFlags().CreateBool() line? If so, can you maybe create a helper like
assertUserPermissions := func(t *testing.T, wantIDs []int32) {
/// ....
}That does the whole reposSTore, provider setup etc. and then call it in each sub-test?
Or: why do we even need to duplicate it? Couldn't we just pull the setup with the external service etc. out? Do you need a fresh DB for both tests? Or could you also sync the permissions once, turn feature flag on, sync again, check that correct permissions are in DB?
mrnugget
left a comment
There was a problem hiding this comment.
Looks good, but I think the tests could be cleaner still.
| // This integration tests performs a user-centric permissions syncing against | ||
| // https://gitlab.sgdev.org, then check if permissions are correctly granted for the test | ||
| // user "sourcegraph-vcr". | ||
| t.Run("test feature flag", func(t *testing.T) { |
There was a problem hiding this comment.
| t.Run("test feature flag", func(t *testing.T) { | |
| t.Run("test gitLabProjectVisibilityExperimental feature flag", func(t *testing.T) { |
| t.Run("feature-flag disabled", func(t *testing.T) { | ||
| assertUserPermissions(t, []int32{2}) | ||
| }) | ||
|
|
||
| t.Run("feature-flag enabled", func(t *testing.T) { | ||
| _, err = testDB.FeatureFlags().CreateBool(ctx, "gitLabProjectVisibilityExperimental", true) | ||
| require.NoError(t, err, "feature flag creation failed") | ||
|
|
||
| assertUserPermissions(t, []int32{1, 2}) | ||
| }) |
There was a problem hiding this comment.
I'd personally remove the t.Run here. t.Run always suggests to me that these tests are independent and can be run independently. But in this case, if the 2nd test runs before the first one, the first one will fail, because "global" state has been modified.
So I'd personally do something like this:
| t.Run("feature-flag disabled", func(t *testing.T) { | |
| assertUserPermissions(t, []int32{2}) | |
| }) | |
| t.Run("feature-flag enabled", func(t *testing.T) { | |
| _, err = testDB.FeatureFlags().CreateBool(ctx, "gitLabProjectVisibilityExperimental", true) | |
| require.NoError(t, err, "feature flag creation failed") | |
| assertUserPermissions(t, []int32{1, 2}) | |
| }) | |
| // Without the featuer flag, we should only get one repository back: | |
| assertUserPermissions(t, []int32{2}) | |
| // But with the feature flag enabled, user should have access to both of them: | |
| _, err = testDB.FeatureFlags().CreateBool(ctx, "gitLabProjectVisibilityExperimental", true) | |
| require.NoError(t, err, "feature flag creation failed") | |
| assertUserPermissions(t, []int32{1, 2}) |
That makes it clearer, IMHO, that the tests are dependent on the same state and that the order is important.
(cherry picked from commit a29b68f)
The FetchUserPermissions code for GitLab never had the feature flags added to the context, so the feature flag was not working as intended.
Loom:
https://www.loom.com/share/646c86923b7f4d70ad6e881b67794f6a
Test plan
Tests passing. Added GitLab integration tests to test feature flag.