authz/azuredevops: Check enforcePermissions in NewAuthzProviders#48549
Conversation
| invalidConnections []string | ||
| problems []string | ||
| warnings []string |
There was a problem hiding this comment.
Now we test for these in the newly added top level test TestProvider_NewAuthzProviders. Previously we were testing implicitly for NewAuthzProviders in this test itself, but it's better to have a separate test.
| { | ||
| name: "licensing feature disabled", | ||
| input: input{ | ||
| mockCheckFeature: func(_ licensing.Feature) error { | ||
| return errors.New("not allowed") | ||
| }, | ||
| connection: &schema.AzureDevOpsConnection{}, | ||
| }, | ||
| output: output{ | ||
| invalidConnections: []string{"azuredevops"}, | ||
| problems: []string{"not allowed"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Moved up to the new test.
| } | ||
|
|
||
| type input struct { | ||
| mockCheckFeature func(licensing.Feature) error |
There was a problem hiding this comment.
All the unit tests in this want the licensing check to pass. Removed it as an input and explicitly setting it below at the start of testing.
| invalidConnections []string | ||
| problems []string | ||
| warnings []string | ||
| providers []authz.Provider |
There was a problem hiding this comment.
This was the same for all the tests - expectedProvidrers. Stopped passing it as an arg and instead using expectedProviders directly.
| connection: &schema.AzureDevOpsConnection{ | ||
| Orgs: []string{"solarsystem"}, | ||
| Projects: []string{"solar/system"}, | ||
| EnforcePermissions: true, |
There was a problem hiding this comment.
Needed to start setting this for the tests to pass as a result of the new change in this PR.
| conf.Mock(nil) | ||
| }) | ||
|
|
||
| licensing.MockCheckFeature = tc.mockCheckFeature |
There was a problem hiding this comment.
Set this once for all the unit tests here.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 80a3b67...65846fe.
|
| if len(conns) == 0 { | ||
| return initResults | ||
| } | ||
| validAzureDevOpsConnections := []*types.AzureDevOpsConnection{} |
There was a problem hiding this comment.
Naming is hard, maybe call this authorizedConnections instead? Or connectionsWithPermissions. Valid evokes in me as if some other config was invalid :)
There was a problem hiding this comment.
I like that name. Going ahead with authorizedConnections. 👍
We do not want to run permission sync for orgs and projects that are declared in code host connections without an explicit
enforcePermissions: true. Skip them instead.Follow up to https://github.com/sourcegraph/sourcegraph/pull/48209#discussion_r1122990998.
Fixes #47165.
Note to reviewers
The code change itself is small but important. Most of the added and removed lines is new tests and some refactoring of the tests.
I've added inline comments to explain the changes which should make it reviewing this more approachable.
Test plan