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

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

Merged

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented May 31, 2024

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};
Copy link
Contributor Author

@sunshowers sunshowers Jul 5, 2024

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
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
@@ -3955,6 +3947,14 @@
"last_port"
]
},
"ProbeExternalIpKind": {
Copy link
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
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 on lines +28 to +29
bstr-6f8ce4dd05d13bba = { package = "bstr", version = "0.2.17" }
bstr-dff4ba8e3ae991db = { package = "bstr", version = "1.9.1" }
Copy link
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
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
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
22 checks passed
@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