- Notifications
You must be signed in to change notification settings - Fork1k
feat(site): display warning messages when wildcard is not configured#19660
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
Could we also highlight the specific apps that have |
site/src/pages/TemplateVersionEditorPage/WildcardHostnameWarning.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
|
…late and workspace pages
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.
Can we adjust verbiage to be similar to what I suggested here.#19660 (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.
Looks good to me from a UX perspective.
| One or more apps in this workspace have subdomain = true, which | ||
| requires a Coder deployment with a Wildcard Access URL configured. | ||
| Please contact your administrator. |
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.
| Oneormoreappsinthisworkspacehavesubdomain=true,which | |
| requiresaCoderdeploymentwithaWildcardAccessURLconfigured. | |
| Pleasecontactyouradministrator. | |
| Oneormoreappsinthisworkspacehave`subdomain = true`,which | |
| requiresaCoderdeploymentwithaWildcardAccessURLconfigured. | |
| Pleasecontactyouradministrator. |
Not sure if we support markdown here, but it would be nice.
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
@david-fraley, can you take a look if we need to update the docs for this? As we are taking the user to docs page. |
|
I agree, it does seem like our docs need some updating. I'll add it to my list. We've gotta focus on the why |
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.
from a design perspective: it seems a bit noisy to have such a big warning while the agent still has a green border.
from a code perspective: I don't think regex is the right way to detect this in the template editor. the regex is very hard to read and not particularly reliable.
when we run the template version build we get back all of the resources from the plan in this query:
coder/site/src/pages/TemplateVersionEditorPage/TemplateVersionEditorPage.tsx
Lines 68 to 71 inf571730
| constresourcesQuery=useQuery({ | |
| ...resources(activeTemplateVersion?.id??""), | |
| enabled:activeTemplateVersion?.job.status==="succeeded", | |
| }); |
...which can be poked at directly. for example:
consta=resourcesQuery.data?.[0]?.agents?.[0].apps?.[0].subdomain;// ^? const a: boolean | undefined
It's a bit of a mess with?. operators to handle all the potentialundefined from this far down, but hopefully it gets the point across. I think using this approach would give more reliable results and would make the code easier to understand.
| <AlertDetail> | ||
| <div> | ||
| This template contains coder_app resources with{" "} | ||
| <MemoizedInlineMarkdown>`subdomain = true`</MemoizedInlineMarkdown>, |
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.
this is a misuse of this component imo. if you look at the source of it, you can see the html that we generate for inline code snippets:
coder/site/src/components/Markdown/Markdown.tsx
Lines 86 to 97 inbd6e91e
| <code | |
| css={(theme)=>({ | |
| padding:"1px 4px", | |
| background:theme.palette.divider, | |
| borderRadius:4, | |
| color:theme.palette.text.primary, | |
| fontSize:14, | |
| })} | |
| {...props} | |
| > | |
| {children} | |
| </code> |
we could just use similar styles and the<code> tag here directly instead:
<codeclassName="py-px px-1 bg-surface-tertiary rounded-sm text-content-primary">subdomain=true</code>
| <MemoizedInlineMarkdown> | ||
| `--wildcard-access-url` | ||
| </MemoizedInlineMarkdown>{" "} |
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.
same here
| </div> | ||
| <div className="flex items-center gap-2 flex-wrap mt-2"> | ||
| <Link href={docs("/admin/setup#wildcard-access-url")} target="_blank"> | ||
| <span css={{ fontWeight: 600 }}> |
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.
| <spancss={{fontWeight:600}}> | |
| <spanclassName="font-semibold"> |
please use tailwind styles
| <AlertDetail> | ||
| <div> | ||
| One or more apps in this workspace have{" "} | ||
| <MemoizedInlineMarkdown>`subdomain = true`</MemoizedInlineMarkdown>, |
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.
same here
| const hasSubdomainCoderApp = Object.keys(fileTree).some((filePath) => { | ||
| const content = fileTree[filePath]; | ||
| return typeof content === "string" && regex.test(content); | ||
| }); |
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.
useObject.values instead. you're not using the key other than to look up the value.
| css={{ | ||
| borderRadius: 0, | ||
| border: 0, | ||
| borderBottom: `1px solid ${theme.palette.divider}`, | ||
| borderLeft: `2px solid ${theme.palette.warning.main}`, | ||
| }} |
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.
pls convert to tailwind
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.
+1 for Kayla's comments. The current visual design feels really noisy. One idea is to try removing the green border when warnings like these get displayed. But right now, the two accent colors are really fighting each other
| const regex = | ||
| /resource\s+"coder_app"\s+"[^"]+"\s*\{[\s\S]*?\bsubdomain\s*=\s*true\b[\s\S]*?\}/s; |
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.
Kayla already flagged the concerns around the regex, but in general, if the regex pattern is 100% static, it can be defined outside the component so it doesn't keep getting recreated on each render
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.
But also, I think this is a case where the variable name could be a bit more specific
| which requires a Coder deployment with a Wildcard Access URL | ||
| configured. Please contact your administrator. | ||
| </div> | ||
| <div className="mt-2"> |
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.
Nit: margins are an anti-pattern 95% of the time in UI programming. Replacing this withpt-2 would have the same visual effect, without any risks of layout side effects
@aslilac I totally agree! I messed up by testing with a scratch template that didn’t have provider/compute resources 🤦 , and because of that, I thought agent resources only showed up after the workspace was created. I've applied all the suggestions 😆 |
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.
thanks for the changes! looking way better now
one last concern tho, we've now got two files called WildcardHostnameWarning.tsx with very similar contents. it seems like it'd be pretty easy to consolidate them. maybe in modules/resources/ next to the AgentRow.tsx being edited?
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.
one more one last thing that would be great to improve, but I think this is good enough to merge now. 😄
| , which requires a Coder deployment with a Wildcard Access URL | ||
| configured. Please contact your administrator. |
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.
sorry to keep "one last thing"-ing you but I was kind of hoping there error messages would get merged. I guess it makes sense to not go on about a flag that the user can't set to normal unprivileged users.
but what if we checked...
const{ permissions}=useAuthenticated();if(permissions.editDeploymentConfig){/* ... */}
and then we could show specific instructions to admins on their workspaces rather than telling admins to go contact an admin.
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.
Nice suggestion. We initially had two separate versions or licensed and OSS versions that I requested to merge. But looks like showing a different version to Template Admin would make more sense.
7c8f8f4 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#19606
This PR adds warnings to alert uses when workspace applications with subdomain access won't work due to missing wildcard access url configuration.
Workspace Page warning
This warning is shown when wildcard hostname is not configured and agent has subdomain applications.
User:
Administrator:
Template Editor Warning:
This warning is shown in the output panel (after the user clicks build) when wildcard hostname is not configured and template contains coder_app resource and subdomain is set to true.