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 client authn verification page #929

Merged
merged 9 commits into from
Jun 29, 2022
Merged

Add client authn verification page #929

merged 9 commits into from
Jun 29, 2022

Conversation

plotnick
Copy link
Contributor

@plotnick plotnick commented Jun 10, 2022

@vercel
Copy link

vercel bot commented Jun 10, 2022

@plotnick is attempting to deploy a commit to the Oxide Computer Company Team on Vercel.

To accomplish this, @plotnick needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@david-crespo
Copy link
Collaborator

Forgot to mention, this looks great. You can join the console team any time

@plotnick plotnick marked this pull request as ready for review June 28, 2022 14:52
@david-crespo
Copy link
Collaborator

console won’t automatically generate the new api client for you. you have to run yarn gen-api. this relies on a local copy of oxide.ts so make sure it’s cloned in the same parent dir as console and up to date.

@plotnick
Copy link
Contributor Author

plotnick commented Jun 29, 2022

console won’t automatically generate the new api client for you. you have to run yarn gen-api.

Yeah. I'm getting an error generating the new api here, but weirdly not in oxide.ts:

libs/api/__generated__/Api.ts: SyntaxError: Identifier 'DeviceAuthRequestParams' has already been declared. (2159:17)

See oxidecomputer/oxide.ts#120

@david-crespo
Copy link
Collaborator

I’ll take a look. Most likely the easy fix will be to rename a struct in omicron.

@vercel
Copy link

vercel bot commented Jun 29, 2022

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

Name Status Preview Updated
console-ui-storybook ✅ Ready (Inspect) Visit Preview Jun 29, 2022 at 7:29PM (UTC)

@david-crespo
Copy link
Collaborator

It was as I expected. Slightly embarrassing for oxide.ts, honestly. I'm going to fix it properly so this doesn't happen anymore, but there's a quick fix in oxidecomputer/omicron#1303.

There was a type named DeviceAuthRequestParams in the list of schemas

https://github.com/oxidecomputer/omicron/blob/154a4a6cd623497cbe893c2cd3ea1c241516bc21/openapi/nexus.json#L6296-L6307

and we generate types for those by default. But the generator also constructs its own type with the same name out of the method called device_auth_request (converted to pascal case) + "Params":

https://github.com/oxidecomputer/oxide.ts/blob/318ad7efc8b3e87eebea0952d3a01addf51d565c/generator/gen-client.ts#L218-L220

So the quick fix in that PR is to rename DeviceAuthRequestParams and friends to DeviceAuthRequest so there's no conflict. The real fix will be to put generated types in their own namespace so it doesn't matter if they're the same as types the API gives us directly.

@david-crespo
Copy link
Collaborator

In df04b09 I generated the client with OMICRON_VERSION pointing at oxidecomputer/omicron#1303 to prove it works. Once that is merged in Omicron we can update this to point to a commit on main.

@david-crespo
Copy link
Collaborator

2022-06-29-gh-auth-quick-polish

2022-06-29-gh-auth-quick-polish-error

# Conflicts:
#	OMICRON_VERSION
#	libs/api/__generated__/OMICRON_VERSION
@plotnick
Copy link
Contributor Author

Sweeeet! Thanks!

@david-crespo david-crespo deleted the client-authn branch June 29, 2022 19:31
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