chore(scripts/rules.go): broaden scope of testingWithOwnerUser linter #10548
Conversation
a992b49 to
8e04dfc
Compare
45417f6 to
c342498
Compare
There was a problem hiding this comment.
Nice improvement! I'm happy to see other feedback, but it is 👍 👍 from me.
The change most likely to be controversial is b) above.
I wouldn't say so. Actually this is specific to coder source, and that's why you described the linter rules this way.
We don't necessarily want to enforce this on the AGPL code yet, but we could do so in future.
This might be opinionated as I like this rule, but maybe we could enable it in a follow up instead of one day in the future?
100% - it will be a big diff though! |
| @@ -787,6 +806,14 @@ func UpdateActiveTemplateVersion(t testing.TB, client *codersdk.Client, template | |||
| require.NoError(t, err) | |||
| } | |||
|
|
|||
| // UpdateTemplateMeta updates the template meta for the given template. | |||
| func UpdateTemplateMeta(t testing.TB, client *codersdk.Client, templateID uuid.UUID, meta codersdk.UpdateTemplateMeta) codersdk.Template { | |||
There was a problem hiding this comment.
With a helper like this, would it make sense to ensure that all methods of codersdk.Client are implemented here instead of just select ones? This essentially turns 2 lines into 1 which arguably doesn't add a whole lot of value if you still don't know when to use client directly, and when to use coderdtest.
There was a problem hiding this comment.
The linter won't complain about passing the owner client into a coderdtest method, which I view as both a feature and a semantic difference:
coderdtest.X: this thing is part of test setup.client.X: this is the thing we are actually testing where ensuring it works with the least-privileged RBAC role is crucial.
I don't think a 1:1 mapping of client methods -> coderdtest methods is warranted, especially as we have some coderdtest methods that call multiple client methods.
This PR broadens the scope of the linter added in #10133.
This would likely have helped catch #10547.
You can just read this commit by commit.
Summary of changes:
testingWithOwnerUserrule to detect:a) Passing client from
coderdenttest.Newtoclitest.SetupConfigb) Usage of any method of the owner client from
coderdenttest.New.CreateGroupandUpdateTemplateMeta.check_enterprise_import.shto ignorescripts/rules.go.The change most likely to be controversial is b) above.
For the enterprise code I feel that it is more critical that we get RBAC correct.
Therefore, any usage of the owner user (excluding some well-known methods) must be justified via
//nolintcomment.We don't necessarily want to enforce this on the AGPL code yet, but we could do so in future.