Skip to content
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

Add VPC, Subnet to NIC table #1047

Merged
merged 11 commits into from
Jul 21, 2022
Merged

Add VPC, Subnet to NIC table #1047

merged 11 commits into from
Jul 21, 2022

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Jul 13, 2022

image

This PR adds VPC and Subnet to the NIC table. This was blocked for a while b/c we only get IDs back and we needed oxidecomputer/omicron#1266 to make it happen.

I added a new cell type that looks up a resource by id and returns whatever field you specify. Currently the cell only renders text, so that'll be something to keep in mind. I don't think this is the "final" version of what this should look like which is why I didn't put more effort in making it robust. The external types work well, but the internal types are... not great.

<Column header="vpc" accessor="vpcId" cell={byIdCell('vpcViewById', 'name')} />
<Column
header="subnet"
accessor="subnetId"
cell={byIdCell('vpcSubnetViewById', 'name')}
/>

@vercel
Copy link

vercel bot commented Jul 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
console-ui-storybook ✅ Ready (Inspect) Visit Preview Jul 21, 2022 at 8:34AM (UTC)

Comment on lines 19 to 20
const { data } = useApiQuery<M>(type, { id: value })
return (data && <span className="text-default">{data[field]}</span>) || null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outside types are correct, but these inner types are fucked.

@zephraph
Copy link
Contributor Author

zephraph commented Jul 13, 2022

I would like to make the vpc/subnet names links eventually. We can do that w/ the VPC without too much trouble, but the subnet is a trick b/c it requires the VPC name. Also subnets are only really viewable inside a list in the VPC so it's hard to focus on a particular table instance.

A side thought here: If we could use UUIDs as a valid param in the route (which would admittedly take some magic to translate correctly) then we could always use the ID in the route path.

@zephraph zephraph marked this pull request as ready for review July 15, 2022 18:17
@david-crespo
Copy link
Collaborator

I tried a version that's more manual but it avoids the @ts-expect-error and made it easy to make the VPC one into a link. See what you think: #1063

type: M
field: keyof NonNullable<AsyncReturnType<ApiViewByIdMethods[M]>['data']>
value: string
}
Copy link
Collaborator

@david-crespo david-crespo Jul 21, 2022

Choose a reason for hiding this comment

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

I spent some time trying to get the approach that works for useApiQuery to work here — no dice. So annoying.

-interface IdLookupCellProps<M extends keyof ApiViewByIdMethods> {
+interface IdLookupCellProps<A extends ApiViewByIdMethods, M extends keyof A> {
   type: M
-  field: keyof NonNullable<AsyncReturnType<ApiViewByIdMethods[M]>['data']>
+  field: keyof NonNullable<AsyncReturnType<A[M]>['data']>
   value: string
 }
-function IdLookupCell<M extends keyof ApiViewByIdMethods>({
+
+function IdLookupCell<A extends ApiViewByIdMethods, M extends keyof A>({
   type,
   field,
   value,
-}: IdLookupCellProps<M>) {
-  // @ts-expect-error TODO M isn't correctly narrowing the type down
-  const { data } = useApiQuery<M>(type, { id: value })
+}: IdLookupCellProps<A, M>) {
+  const { data } = useApiQuery<M>(type as M, { id: value })
   // @ts-expect-error TODO Because data isn't narrowed correctly above this is an error too
   return (data && <span className="text-default">{data[field]}</span>) || null
 }
 
-export const byIdCell =
-  <M extends keyof ApiViewByIdMethods>(
-    type: IdLookupCellProps<M>['type'],
-    field: IdLookupCellProps<M>['field']
+const getByIdCell =
+  <A extends ApiViewByIdMethods>() =>
+  <M extends keyof A>(
+    type: IdLookupCellProps<A, M>['type'],
+    field: IdLookupCellProps<A, M>['field']
   ) =>
   ({ value }: Cell<string>) => {
     return <IdLookupCell type={type} field={field} value={value} />
   }
+
+export const byIdCell = getByIdCell()

@zephraph
Copy link
Contributor Author

zephraph commented Jul 21, 2022

Cool, yeah. Manual approach works 👍. The subnet link is what was more difficult given you need the name of the vpc. Fine with doing without that for now.

@zephraph zephraph deleted the add-nic-details branch July 21, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants