Skip to content

feat: expose owner_name in coder_workspace resource#11639

Merged
mtojek merged 18 commits into
mainfrom
9883-add-owner-to-workspace
Jan 17, 2024
Merged

feat: expose owner_name in coder_workspace resource#11639
mtojek merged 18 commits into
mainfrom
9883-add-owner-to-workspace

Conversation

@mtojek

@mtojek mtojek commented Jan 16, 2024

Copy link
Copy Markdown
Member

Related: #9883

This PR modifies the users database table to store the workspace owner's name (first name, last name).

Follow-up:

  • Adjust the Account form to let the user customize their name
  • Adjust frontend tests with "" names

@mtojek mtojek self-assigned this Jan 16, 2024
@mtojek mtojek mentioned this pull request Jan 16, 2024
3 tasks
@mtojek mtojek marked this pull request as ready for review January 17, 2024 10:49
@mtojek mtojek requested review from johnstcn and mafredri January 17, 2024 10:49

@johnstcn johnstcn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Names are hard!

Comment thread coderd/database/migrations/000185_add_user_name.up.sql
Comment thread coderd/httpapi/name.go Outdated
Comment thread coderd/httpapi/name.go Outdated
Comment thread coderd/httpapi/name_test.go
Comment thread coderd/httpapi/name.go

// UserRealNameValid returns whether the input string is a valid real user name.
func UserRealNameValid(str string) error {
if len(str) > 128 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the motivation for this limit? Hubert will be sad: https://www.guinnessworldrecords.com/world-records/67285-longest-personal-name

I think it's unlikely we'll hit this, but arbitrary limits have a way of making people sad (think email regex). We should at least document this decision a bit better. Should length also be enforced in the DB side?

PS. If we're thinking about frontend, they could always use ellipse over there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would expect we don't want to allow someone to fill up the database with a name that's gigabytes in size.

@mafredri mafredri Jan 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I understand that risk could be a concern, I was specifically thinking about this limit (128), though. Seems on the smaller to just be a safety-net. And if we're really worried about this case, I do think database limits would be a good idea, otherwise we'll always run the risk of introducing new holes. And I'd be surprised if we didn't have some gaps in this defense elsewhere currently as well 😄.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The form Mr. Hubert used, HUBERT BLAINE WOLFE­SCHLEGEL­STEIN­HAUSEN­BERGER­DORFF SR, is within the limit!

The original use case for this feature is to populate all places where UI or CLI can render it, like Git commiter/message. I'm sure that Hubert Blain Wolf... will crash UIs in many different places, so I would stick to the artificial limit.

Anyway, we can always disable the limit. The other way round might be hard to do as we might need to chop some names...

I do think database limits would be a good idea, otherwise we'll always run the risk of introducing new holes. And I'd be surprised if we didn't have some gaps in this defense elsewhere currently as well 😄.

This is valid risk, and I wouldn't like to enable another gate to pump bytes into the database.

{"Hector Ó hEochagáin", true},
{"Małgorzata Kalinowska-Iszkowska", true},
{"成龍", true},
{". .", true},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a smiley, I approve! 👀

@mtojek mtojek merged commit 5eb3e1c into main Jan 17, 2024
@mtojek mtojek deleted the 9883-add-owner-to-workspace branch January 17, 2024 12:20
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 17, 2024
Comment thread coderd/httpapi/name.go
}

if strings.TrimSpace(str) != str {
return xerrors.New("must not have leading or trailing white spaces")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Happened to catch my eye, whitespace is uncountable, so we drop s. Alt: whitespace characterS.

Suggested change
return xerrors.New("must not have leading or trailing white spaces")
return xerrors.New("must not have leading or trailing whitespace")

Not sure about having white space in whitespace, though. Both could be correct 😄.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants