[RFC] [gateway-types] reorganize per RFD 619

[sunshowers]

No description provided.

[jgallagher]

I'm surprised to see explicit versions in these types. I thought:

• The *-versions crate would have a pub use latest::*
• All the "current" endpoints would reference the top-level (latest) types, and only endpoints from older versions would reference explicitly-versioned types

[sunshowers]

We've discussed and addressed this -- latest versions now use floating identifiers.

[jgallagher]

Thanks, I like how this is all shaping up quite a lot. Just some small style notes/preferences.

[jgallagher]

It seems a little weird that from inside what looks like the initial module, I have to refer to other submodules of myself as v1. Not a huge deal, but a downside of our #[path = "..."] plan.

Would this be less confusing as use super::component::SpIdentifier?

[jgallagher]

Might be worth adding comments in each of the *-versions crates' lib.rs giving a brief summary of guidance, similar to

omicron/nexus/db-model/src/schema_versions.rs

Lines 21 to 31 in 4e6a568

	/// List of all past database schema versions, in *reverse* order
	///
	/// If you want to change the Omicron database schema, you must update this.
	static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
	vec![
	// +- The next version goes here! Duplicate this line, uncomment
	// | the *second* copy, then update that copy for your version,
	// | leaving the first copy as an example for the next person.
	// v
	// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
	KnownVersion::new(212, "local-storage-disk-type"),

Kinda sucks that this would be copy-pasted a bunch of times since we have a bunch of crates, but I think that might be worth it? Any given change is likely to only touch a small number of them, and it'd be nice to have the guidance right there in line.

[jgallagher]

This is purely a style choice, but I think I would have imported all of the latest types so that this remained

    async fn sp_get(
        rqctx: RequestContext<Self::Context>,
        path: Path<PathSp>,
    ) -> Result<HttpResponseOk<SpState>, HttpError> {

and maybe would have done the same even in the *-api crate? And then only use explicit (non-latest) versions on the endpoints that need them.

I think this is mostly because adding latest::submodule:: to every type referenced in this file causes a lot of pretty long lines and makes this diff noisier than it would be otherwise. It seems fine to me for an unadorned type to implicitly be "the latest". But I don't feel as strongly about this as I did other things we've already discussed.

[andrewjstone]

I have mixed opinions here. For new hires, and people not in the code a bunch, the types explicitly being marked latest could be informative at a glance. On the other hand I sympathize pretty heavily with eliminating the repetition and long lines.

I think I prefer John's suggestion, and would just add a comment to the import about unversioned types being latest.

One slight wrinkle is that some type names may conflict on their own. In that case though, the module can still be the prefix or we can use an alias. Nothing really changes how that currently works.