Skip to content

refactor: increase group name limit to 255 #15377

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 6, 2024
Merged

refactor: increase group name limit to 255 #15377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64d2a75
feat(coderd): increase group name limit to 255
BrunoQuaresma Nov 5, 2024
950a903
Merge branch 'main' of https://github.com/coder/coder into bq/feat-in…
BrunoQuaresma Nov 6, 2024
60cb055
Apply review suggestions
BrunoQuaresma Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions codersdk/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package codersdk

import (
"fmt"
"regexp"
"strings"

Expand Down Expand Up @@ -98,9 +99,12 @@ func UserRealNameValid(str string) error {

// GroupNameValid returns whether the input string is a valid group name.
func GroupNameValid(str string) error {
// 36 is to support using UUIDs as the group name.
if len(str) > 36 {
return xerrors.New("must be <= 36 characters")
// We want to support longer names for groups to allow users to sync their
// group names with their identity providers without manual mapping. Related
// to: https://github.com/coder/coder/issues/15184
limit := 255
if len(str) > limit {
return xerrors.New(fmt.Sprintf("must be <= %d characters", limit))
}
// Avoid conflicts with routes like /groups/new and /groups/create.
if str == "new" || str == "create" {
Expand Down
39 changes: 39 additions & 0 deletions codersdk/name_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package codersdk_test
import (
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/exp/rand"

"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/testutil"
Expand Down Expand Up @@ -254,3 +256,40 @@ func TestUserRealNameValid(t *testing.T) {
})
}
}

func TestGroupNameValid(t *testing.T) {
t.Parallel()

testCases := []struct {
Name string
Valid bool
}{
{"", false},
{"my-group", true},
{"create", false},
{"new", false},
Copy link
Member

Choose a reason for hiding this comment

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

Why is this invalid?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)
}

{"Lord Voldemort Team", false},
{randomString(255), true},
{randomString(256), false},
Copy link
Member

Choose a reason for hiding this comment

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

Could you use testutil.GetRandomName(...) ?

Copy link
Member

Choose a reason for hiding this comment

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

I think get random name is limited in length 🤔

Copy link
Member

Choose a reason for hiding this comment

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

oh, then perhaps namesgenerator.GetRandomName(255)?

Copy link
Member

Choose a reason for hiding this comment

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

GetRandomName only checks if the argument is greater than 0, and if it is, appends a random number between 1 and 10

Copy link
Member

Choose a reason for hiding this comment

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

ok, one last guess: cryptorand.String(...)

}
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.Name, func(t *testing.T) {
t.Parallel()
err := codersdk.GroupNameValid(testCase.Name)
assert.Equal(t, testCase.Valid, err == nil)
Copy link
Member

Choose a reason for hiding this comment

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

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)

})
}
}

const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"

// RandomString generates a random string of a given length.
func randomString(length int) string {
seededRand := rand.New(rand.NewSource(uint64(time.Now().UnixNano())))
result := make([]byte, length)
for i := range result {
result[i] = charset[seededRand.Intn(len(charset))]
}
return string(result)
}
Loading