refactor: increase group name limit to 255 #15377
Conversation
| {"", false}, | ||
| {"my-group", true}, | ||
| {"create", false}, | ||
| {"new", false}, |
There was a problem hiding this comment.
To avoid route conflicts.
// Avoid conflicts with routes like /templates/new and /groups/create.
if str == "new" || str == "create" {
return xerrors.Errorf("cannot use %q as a name", str)
}|
@ethanndickson @code-asher I see you both worked on this group name validation in the past, so I'm just checking if my changes are ok. |
codersdk/name_test.go
Outdated
| {randomString(255), true}, | ||
| {randomString(256), false}, |
There was a problem hiding this comment.
Could you use testutil.GetRandomName(...) ?
There was a problem hiding this comment.
I think get random name is limited in length 🤔
There was a problem hiding this comment.
oh, then perhaps namesgenerator.GetRandomName(255)?
There was a problem hiding this comment.
GetRandomName only checks if the argument is greater than 0, and if it is, appends a random number between 1 and 10
There was a problem hiding this comment.
ok, one last guess: cryptorand.String(...)
codersdk/name_test.go
Outdated
| t.Run(testCase.Name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := codersdk.GroupNameValid(testCase.Name) | ||
| assert.Equal(t, testCase.Valid, err == nil) |
There was a problem hiding this comment.
Can you modify this to contain the error if something bad happens:
assert.Equal(t, testCase.Valid, err == nil, "Test case %s failed: expected valid=%t but got error: %v", testCase.Name, testCase.Valid, err)
…crease-group-name-length
Close #15184