- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: implement sign up with GitHub for the first user#16629
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5fb6932 toa4258c7Compareb16682c to2473a5eComparedab3105 tobe8cca1Compare2473a5e to1055883Comparebe8cca1 tocfc3645Compare1055883 to484afd4Compare484afd4 to9549df5Comparehugodutka commentedFeb 21, 2025
By the way, you can test the changes locally by starting the coder server like this: |
dannykopping commentedFeb 24, 2025
PS Once I get a full build running and auth with your client ID I get:
|
dannykopping commentedFeb 24, 2025
I tested using |
hugodutka commentedFeb 24, 2025
@dannykopping yes - only the first user can be created by default. This matches how Coder works currently. Tu use |
dannykopping commentedFeb 24, 2025
Sweet 👌 works nicely |
Emyrk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, I did not test it though
| OrganizationIDs: []uuid.UUID{defaultOrg.ID}, | ||
| }, | ||
| LoginType:database.LoginTypePassword, | ||
| RBACRoles: []string{rbac.RoleOwner().String()}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
❤️
| onChangeTrimmed(form)(event); | ||
| }, | ||
| [form], | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO, there is no need to useuseCallback since the function is not used as a dependency in an effect and the function doesn't look heavy for optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
moved it out ofuseCallback
| <divclassName="shrink-0 text-xs uppercase text-content-secondary tracking-wider"> | ||
| or | ||
| </div> | ||
| <divclassName="h-[1px] w-full bg-divider"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't use the token divider so you could usebg-border-default. Let me know in case it does not look good.
hugodutkaFeb 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added support for the divider token to mirror the color palette of the sign in screen exactly. Would you like me to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes please, since we don't use it in our design system.
hugodutkaFeb 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done!bg-border-default isn't valid, butbg-border is and it looks good.
1a67ae7 todfd49d0Comparedfd49d0 to3b36025Compare67d89bb intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Second PR to address#16230. See the issue for more context and discussion.
It adds a "Continue with GitHub" button to the
/setuppage, so the deployment's admin can sign up with it. It also removes the "Username" and "Full Name" fields to make signing up with email faster. In the email flow, the username is now auto-generated based on the email, and full name is left empty.There's a separate, follow up issue to visually align the
/setuppage with the new design system:#16653