Skip to content

[nexus] switch nexus internal API to a trait, add scaffolding to manage it#5844

Merged
sunshowers merged 16 commits into
mainfrom
sunshowers/spr/example-conversion-of-nexus-internal-api
Jul 12, 2024
Merged

[nexus] switch nexus internal API to a trait, add scaffolding to manage it#5844
sunshowers merged 16 commits into
mainfrom
sunshowers/spr/example-conversion-of-nexus-internal-api

Conversation

@sunshowers

@sunshowers sunshowers commented May 31, 2024

Copy link
Copy Markdown
Contributor

Part of the work on RFD 479 Dropshot API traits.

As a first, impactful example, this PR switches over the Nexus internal API to being defined by a trait. Some minor refactoring was required but overall it took me around half an hour to do this.

Included also is a tool, cargo xtask openapi, which is used to manage this and future documents, as well as instructions that folks can hopefully follow.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the base branch from main to sunshowers/spr/main.example-conversion-of-nexus-internal-api July 4, 2024 04:49
@sunshowers sunshowers changed the title example conversion of nexus internal API [nexus] switch nexus internal API to a trait, add scaffolding to manage it Jul 4, 2024
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the base branch from sunshowers/spr/main.example-conversion-of-nexus-internal-api to main July 5, 2024 21:39
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers marked this pull request as ready for review July 5, 2024 22:23
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use std::{io::Write, process::ExitCode};

@sunshowers sunshowers Jul 5, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A lot of this code is quite generic, and I think can live separately so it can be used by other projects. What do you think about moving this out to the dropshot repo or its own repo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In a meeting yesterday we decided to wait and see -- leave it in omicron for now but be open to the possibility of putting this code in a central location later.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1

[skip ci]
Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the base branch from main to sunshowers/spr/main.example-conversion-of-nexus-internal-api-1 July 11, 2024 21:48
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
"last_port"
]
},
"ProbeExternalIpKind": {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

was this renamed on purpose?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah -- there's already an ExternalIp type at

pub enum ExternalIp {
which is exported by the Nexus external API. Basically we have several types all called something like ExternalIp that are mostly but not entirely isomorphic (e.g. some of them carry around database UUIDs). We should probably revisit this at some point.

Comment thread workspace-hack/Cargo.toml
Comment on lines +28 to +29
bstr-6f8ce4dd05d13bba = { package = "bstr", version = "0.2.17" }
bstr-dff4ba8e3ae991db = { package = "bstr", version = "1.9.1" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this intentional or do we want a single bstr?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hah good question -- this happened because I pulled in the similar library to do diffs, which depends on bstr 0.2. I filed mitsuhiko/similar#66 about this.

@sunshowers

Copy link
Copy Markdown
Contributor Author

Will fix the test failures and rebase the PR onto main once #6056 lands.

@sunshowers sunshowers changed the base branch from sunshowers/spr/main.example-conversion-of-nexus-internal-api-1 to main July 12, 2024 00:00
Created using spr 1.3.6-beta.1
@sunshowers sunshowers enabled auto-merge (squash) July 12, 2024 00:00
Created using spr 1.3.6-beta.1
@sunshowers sunshowers merged commit bedb238 into main Jul 12, 2024
@sunshowers sunshowers deleted the sunshowers/spr/example-conversion-of-nexus-internal-api branch July 12, 2024 17:37
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