-
Notifications
You must be signed in to change notification settings - Fork 62
Collect internal DNS generation in inventory #8643
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
Conversation
jgallagher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update the inventory display implementation? Should be easy now that #8613 has landed.
c16686f to
bea6e02
Compare
a7dca86 to
e7f1843
Compare
bea6e02 to
5f2c4dd
Compare
1fc3373 to
9fda2da
Compare
Updated nexus/types/src/inventory/display.rs to include this |
1c5a033 to
189ec3d
Compare
| keeper_admin_clients: Vec<clickhouse_admin_keeper_client::Client>, | ||
| cockroach_admin_client: &'a CockroachClusterAdminClient, | ||
| ntp_admin_clients: Vec<(OmicronZoneUuid, ntp_admin_client::Client)>, | ||
| dns_service_clients: Vec<(OmicronZoneUuid, dns_service_client::Client)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't change this on this PR (or maybe at all), but just thinking out loud - would it make sense for ntp_admin_client and dns_service_client to wrap the progenitor Client in something that holds an OmicronZoneUuid? It could impl Deref so you could call all the normal client methods, but also say client.zone_id().
I guess that would force every client to look up the zone ID, which maybe they don't care about. It could expose both the existing Client and a ClientWithZoneId I guess? Maybe that's not really worth it.
789b66f to
506ab20
Compare
506ab20 to
0df7e2b
Compare
Fixes #8544