Skip to content

feat: expose owner_name in coder_workspace resource #11639

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.

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 18 commits into from
Jan 17, 2024

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Jan 16, 2024

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 mafredri and johnstcn January 17, 2024 10:49
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Names are hard!


// UserRealNameValid returns whether the input string is a valid real user name.
func UserRealNameValid(str string) error {
if len(str) > 128 {
Copy link
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
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.

Copy link
Member

@mafredri mafredri Jan 17, 2024

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

if strings.TrimSpace(str) != str {
return xerrors.New("must not have leading or trailing white spaces")
Copy link
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
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