Update pinned omicron version

[zephraph]

This PR updates to the latest omicron version. There's a few caveats.

Once GREsau/schemars#108 and oxidecomputer/omicron#273 were merged our schema generation updated to use oneOf instead of anyOf in cases where we want to represent a discriminated union. Unfortunately the open-api generator we're using didn't interpret the spec change in the way we'd hoped. You can see some of the context on that below.

To combat the undesirable generation I've added a temporary measure of running codemods as a post generation step to result in code closer to what we want. It's still not ideal and we may end up migrating over to swagger-typescript-api eventually, but these changes unblock us now in a way that will continue to work even if new disk states are added.

---

I'll note that as mentioned by @david-crespo below, the JSON marshaling doesn't exactly work in the best way. I'd considered making a more sophisticated codemod to handle that but decided that isn't the best use of my time right now.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/BVVavrWKbmC3JrTYZmetHZZRmcsv
✅ Preview: https://console-ui-storybook-git-update-api-client-oxidecomputer.vercel.app

[github-actions]

Preview will be deployed at https://console-git-update-api-client.internal.oxide.computer

[david-crespo]

looking gooder

[zephraph]

Ish, heh. The nested enum isn't the best. It doesn't really resolve for type completion. It seems like this should be a singular enum instead of what it's producing (a union of single member enums).

[david-crespo]

Yeah, I hadn't seen the nested enum, that's terrible. I don't mind using a discriminated union at top level, but I would want the arms to use strings instead of enums, like { state: 'creating' } | { state: 'detached' } | ...

[zephraph]

Okay, it's updated to do this now. I've added codemods to patch up (most) of the changes we wanted which includes ensuring there's actually a string discriminant instead of a single element enum. See this codemod. It updates it for all oneOf models including SagaErrorInfo and SagaState. Any new models that implement this same pattern will also be patched automatically.

[david-crespo]

this is awful haha. pretty sure the logic is wrong in addition to typecheck failing. 3 returns

{
    instance: value.instance,
    state: value.state,
}

so even if you're on an arm that doesn't have an instance key, you're still going to get instance: undefined in the result.

[david-crespo]

however, doing it right sounds kind of sophisticated because it has to know to use state at runtime as a discriminator to choose which serializer to use. urg

[david-crespo]

turns out I noticed the to/from JSON weirdness and forgot about it
#458 (comment)

Also notable: swagger-typescript-api produces a perfect model for DiskState BUT it provides no special to/from JSON. For parsing responses, it just sticks the response JSON in an object with the type, and doesn't, e.g., convert timestamps to Date. And for serializing to JSON it provides nothing, which is maybe fine since we're not actually using that anyway?
https://github.com/oxidecomputer/console/blob/sta/gen-client/Api.ts#L118-L128

[zephraph]

Yeah, I did some deeper exploration with swagger-typescript-api which definitely does produce better type results. The overall API ergonomics itself become difficult though. It automatically namespaces methods based off of routes which is a little annoying:

console/gen-client/Api.ts

Line 925 in 2d948f0

hardware = {

I eventually figured out a way around that using their internal hooks.

Here's a build script if you want to toy with it: https://gist.github.com/zephraph/db8e8f73a4d9c887a08e838ed6057774

More to the point though, after looking a bit more at the openapi description the output that we're seeing isn't totally unreasonable.

{
  "DiskState": {
    "description": "State of a Disk (primarily: attached or not)",
    "oneOf": [
      {
        "description": "Disk is being initialized",
        "type": "object",
        "properties": {
          "state": {
            "type": "string",
            "enum": ["creating"]
          }
        },
        "required": ["state"]
      }
    ]
  }
}

It's taking the naive approach here and viewing every enum as a unique instance of an enum. Given what I'm seeing from the documentation this is likely the output is likely correct to spec even if it's not what we actually want.

oneOf validates the value against exactly one of the subschemas
-- source

This is perhaps something we'll need to do some custom generation for. I assume that means modifying a template to produce our desired outcome.

[zephraph]

Hmmm, negative on the template. That's more for formatting output than it is for making logical changes.

https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/typescript-fetch

[zephraph]

Okay, so here's the thing... I'm not convinced openapi actually allows us to express what we're wanting to express here. I've been trying to generate the output in other languages just to see what would happen and I'm seeing similar results. If multiple language outputs for the official tool spits out bad results it points more towards our original openapi spec being bad.

Maybe a better exercise for us to go through is what does the openapi spec need to look like for us to get the generated output we want? We can work our way up from there... back to the generator where I feel is ultimately the place we need to do the change.

Ultimately, in my mind, we want the output of the official generator to be correct. If it's not then we'll be fighting the ecosystem and ultimately have a worse SDK experience for our end users.

[david-crespo]

I see, it seems like the spec is bad and we need to fix that. Turns out OpenAPI does support specifying a "discriminator" property, which is exactly what state is functioning as here.

Of course, we'll have to see if the TS generator knows what to do with that. Looks like it might. Note also that the config page mentions a setting for "legacy discriminator behavior".

[david-crespo]

To be more specific about how to improve the spec:

1. It should specify that state is a discriminator

2. This bit should maybe not be an enum (unless that's correct way to say it has one specific value)

   "state": {
     "type": "string",
     "enum": ["creating"]
   }
   

[david-crespo]

Will discuss tomorrow. Inline schemas don't work with discriminator, so each arm of the oneOf would have to be pulled out as its own schema instead of inlined. Also it seems from the docs that you can't just use state as the discriminator, the value of the discriminator has to be the name of the schema. So it would have to be something like DiskState_Creating, etc. Gnarly.

93254f0

[zephraph]

Yeah, I'd tried the discriminator field too last night but the result definitely isn't helpful.

[david-crespo]

This is wild! Never written a codemod before. Very clean.

[zephraph]

They're incredibly useful tools. I've been meaning to write a blog post on it for a while, might do that this weekend.

[david-crespo]

is it too fiddly to get the codemod to do the even more correct thing here? probably, right? like the codemod would basically have to encode the mapping from states to all the subtypes

switch (value.state) {
  case 'creating':
    return DiskStateOneOfToJSON(value)
  case 'detached':
    return DiskStateOneOf1ToJSON(value)
  // etc
}

[david-crespo]

oh haha I just saw your note at the end of the PR description

[zephraph]

I can do it. It'd probably would only take an hour or two. We can follow up on it.