Rename params::AddressSelector to params::AddressAllocator in the external API #9669
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using jj-spr 0.1.0
david-crespo
approved these changes
Jan 16, 2026
| floating_params.map(Into::into), | ||
| ) | ||
| .await | ||
| } |
Contributor
There was a problem hiding this comment.
This stuff is wild. I haven't done one of these changes yet.
Contributor
There was a problem hiding this comment.
Very cool to see in action
Collaborator
There was a problem hiding this comment.
"The way of the future"
Contributor
|
Schema diff generated with api-diff. Will be unnecessary after the RFD 634 changes go through, assuming they do. --- a/2026011500.0.0-9a6768/spec.json
+++ b/2026011600.0.0-736cc9/spec.json
@@ -7,7 +7,7 @@
"url": "https://oxide.computer",
"email": "api@oxide.computer"
},
- "version": "2026011500.0.0"
+ "version": "2026011600.0.0"
},
"paths": {
"/device/auth": {
@@ -13430,6 +13430,59 @@
}
},
"required": ["address", "address_lot"]
+ },
+ "AddressAllocator": {
+ "description": "Specify how to allocate a floating IP address.",
+ "oneOf": [
+ {
+ "description": "Reserve a specific IP address.",
+ "type": "object",
+ "properties": {
+ "ip": {
+ "description": "The IP address to reserve. Must be available in the pool.",
+ "type": "string",
+ "format": "ip"
+ },
+ "pool": {
+ "nullable": true,
+ "description": "The pool containing this address. If not specified, the default pool for the address's IP version is used.",
+ "allOf": [
+ {
+ "$ref": "#/components/schemas/NameOrId"
+ }
+ ]
+ },
+ "type": {
+ "type": "string",
+ "enum": ["explicit"]
+ }
+ },
+ "required": ["ip", "type"]
+ },
+ {
+ "description": "Automatically allocate an IP address from a specified pool.",
+ "type": "object",
+ "properties": {
+ "pool_selector": {
+ "description": "Pool selection.\n\nIf omitted, this field uses the silo's default pool. If the silo has default pools for both IPv4 and IPv6, the request will fail unless `ip_version` is specified in the pool selector.",
+ "default": {
+ "ip_version": null,
+ "type": "auto"
+ },
+ "allOf": [
+ {
+ "$ref": "#/components/schemas/PoolSelector"
+ }
+ ]
+ },
+ "type": {
+ "type": "string",
+ "enum": ["auto"]
+ }
+ },
+ "required": ["type"]
+ }
+ ]
},
"AddressConfig": {
"description": "A set of addresses associated with a port configuration.",
@@ -13665,59 +13718,6 @@
}
},
"required": ["blocks", "lot"]
- },
- "AddressSelector": {
- "description": "Specify how to allocate a floating IP address.",
- "oneOf": [
- {
- "description": "Reserve a specific IP address.",
- "type": "object",
- "properties": {
- "ip": {
- "description": "The IP address to reserve. Must be available in the pool.",
- "type": "string",
- "format": "ip"
- },
- "pool": {
- "nullable": true,
- "description": "The pool containing this address. If not specified, the default pool for the address's IP version is used.",
- "allOf": [
- {
- "$ref": "#/components/schemas/NameOrId"
- }
- ]
- },
- "type": {
- "type": "string",
- "enum": ["explicit"]
- }
- },
- "required": ["ip", "type"]
- },
- {
- "description": "Automatically allocate an IP address from a specified pool.",
- "type": "object",
- "properties": {
- "pool_selector": {
- "description": "Pool selection.\n\nIf omitted, this field uses the silo's default pool. If the silo has default pools for both IPv4 and IPv6, the request will fail unless `ip_version` is specified in the pool selector.",
- "default": {
- "ip_version": null,
- "type": "auto"
- },
- "allOf": [
- {
- "$ref": "#/components/schemas/PoolSelector"
- }
- ]
- },
- "type": {
- "type": "string",
- "enum": ["auto"]
- }
- },
- "required": ["type"]
- }
- ]
},
"AffinityGroup": {
"description": "View of an Affinity Group",
@@ -18068,7 +18068,7 @@
"description": "Parameters for creating a new floating IP address for instances.",
"type": "object",
"properties": {
- "address_selector": {
+ "address_allocator": {
"description": "IP address allocation method.",
"default": {
"pool_selector": {
@@ -18079,7 +18079,7 @@
},
"allOf": [
{
- "$ref": "#/components/schemas/AddressSelector"
+ "$ref": "#/components/schemas/AddressAllocator"
}
]
}, |
mergeconflict
added a commit
that referenced
this pull request
Jan 16, 2026
mergeconflict
added a commit
that referenced
this pull request
Jan 16, 2026
…david-crespo's feedback Created using jj-spr 0.1.0
Created using jj-spr 0.1.0
Created using jj-spr 0.1.0
| // TODO: Add proptests for the conversions in all of the other version | ||
| // modules, and extract common strategies into a separate test-utils module. | ||
| // | ||
| // Alternatively, consider adding `#[cfg_attr(any(test, feature = "testing"), |
Collaborator
There was a problem hiding this comment.
I see this pattern here:
#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))]
pub struct BaseboardId {
...Nonetheless, I like the general idea of proptesting here; however, it also feels like it's testing something innate, i.e. the From implementation copying fields over. So, is it necessary in this form?
I think we'd want some conformance testing suiting for conversions in general? If we did implement Arbitrary, to your point, maybe there's a macro like approach for all conversions we can follow?
Anyway, just spewing some thoughts, but maybe we want to tackle testing separately on the whole? idk.
Contributor
Author
There was a problem hiding this comment.
Yeah I think proptesting has some value here. I'm not trying to boil the ocean in this PR but I did realize (as you noted below) that we need to chain the conversions (RFD 619 suggests it's best to "hop through intermediate versions"). So I figured I'd try to clean that up a bit just for the specific types I'm touching.
Collaborator
|
To @mergeconflict's point in chat, there were mid version change(s) to the same type, so we have to delegate through to each. |
Created using jj-spr 0.1.0
| }, | ||
| }; | ||
| params::FloatingIpCreate { identity: old.identity, address_selector } | ||
| // Converts to v2026010300::FloatingIpCreate (adds ip_version: None) |
Collaborator
There was a problem hiding this comment.
Dope. Good catch.
mergeconflict
deleted the
spr/mergeconflict/rename-paramsaddressselector-to-paramsaddressallocator-in-the-external-api
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From discussion in #9638 (comment), we decided that
AddressSelectoris a bit of a misnomer in our current scheme, where "selector" implies filtering/fetching from existing resources. We'll useAddressAllocatorinstead, which better describes the action of reserving/assigning a floating IP address from a pool.I've also done a bit of refactoring here to be more RFD 619 friendly. Before this PR, we had a series of
FloatingIpCreateversions that each hadFromorTryFromconversions directly to the latest API version in "big step" style. This becomes an O(n)^2 problem to maintain over time. Instead, in this PR, we convert to "small step" style, where each older version ofFloatingIpCreateconverts to the next newer version. In the terms of the RFD, we "hop through intermediate versions." This ensures that the next time we changeFloatingIpCreate, we only need to introduce one new conversion, rather than update all older conversions.I've added proptests to make our expectations of the conversion behavior more explicit and prevent regressions.