-
Notifications
You must be signed in to change notification settings - Fork 15
Shenanigans required for serving console from nexus #532
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
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/7Ge8wTMreba44srfcbDeZ1eXGjVL |
|
Preview will be deployed at https://console-git-serve-from-nexus.internal.oxide.computer |
| "post": { | ||
| "operationId": "spoof_login", | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/LoginParams" | ||
| } | ||
| } | ||
| }, | ||
| "required": true | ||
| }, | ||
| "responses": {} | ||
| } |
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'm assuming this eventually goes away? It seems like while we may still want the capability for dev, we definitely wouldn't want it in the public api spec.
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, that's correct. The demo login form is really only in the console bundle in the first place because of the convenience of using existing styles. Login will really be through the customer's IdP, but If Nexus does end up serving some kind of login form, e.g., for use by admins before the IdP is set up, I think that should be a separate bundle with minimal JS (ideally so minimal we could inline it in the HTML response) and it would not use the generated API client at all. It could even use an HTML form POST.
I'll make a comment about this in the LoginPage file.
| onError: () => { | ||
| addToast({ | ||
| title: 'Bad credentials', | ||
| icon: <Warning12Icon />, |
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'd definitely like to update the icon apis a bit. This was the most expedient implementation, but eventually I'd like to at least have <WarningIcon size={12}/> where size is required and strictly typed.
|
|
||
| <div className="space-x-4"> | ||
| <Link to={`/orgs/${orgName}/projects/new`} className={buttonStyle()}> | ||
| <Link to="projects/new" className={buttonStyle()}> |
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.
Is this because the link is relative to the current location?
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.
Almost, it's relative to the parent node in the route tree, in this case /orgs/:orgName.
Lines 71 to 73 in c1378a6
| <Route path="orgs"> | |
| <Route path=":orgName" element={<RootLayout />} crumb={orgCrumb}> | |
| <Route index element={<OrgPage />} /> |
Originally I did this because it made the link totally indifferent to the /c/ prefix, but even without that it's kind of cool not to have to explicitly build up the full route. On the other hand it's a layer of misdirection because in order to know where this link goes, you have to know what route is rendering this page. So I'm open to changing it back.
tools/start_api.sh
Outdated
| # if [ "$CURRENT_API_VERSION" != "$PINNED_API_VERSION" ]; then | ||
| # echo -e "\nERROR: Omicron version pinned in console does not match HEAD\n" >&2 | ||
| # echo -e " pinned: $PINNED_API_VERSION" >&2 | ||
| # echo -e " HEAD: $CURRENT_API_VERSION\n" >&2 | ||
| # echo -e "Check out the pinned commit and try again.\n" >&2 | ||
| # exit 1 | ||
| # fi |
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.
Why comment this out?
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.
forgot to undo it
just-be-dev
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.
Overall looks good. I'm pretty excited for this!
/loginthat's just buttons formain)Demo