-
Notifications
You must be signed in to change notification settings - Fork 59
Remove bad project_ prefix from silo-scoped IP pool endpoints
#8894
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
base: main
Are you sure you want to change the base?
Conversation
project_ prefix from silo-scoped IP pool endpoints
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.
Thanks for fixing this! It's really nice to have clarity.
openapi/nexus.json
Outdated
| ], | ||
| "summary": "List IP pools", | ||
| "operationId": "ip_pool_list", | ||
| "description": "Operator endpoint to list all IP pools across the system.", |
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.
Does this mean that it will list all IP pools across all silos? If so, maybe the description should clarify. It's not super obvious.
I'm not also a huge fan of the "Operator" term in user facing docs. It may not mean anything to our users, or it may mean something else to them. It really is up to each customer to define who has permissions to what, and it may not always fit neatly into what we think of as an "Operator"
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 think you're right on both counts. "List IP pools across all silos."
|
CC @oxidecomputer/solutions-software-engineering, this will be a breaking change in the Go SDK and the terraform provider will need to be updated as well |
|
@karencfv what do you think about prefixing all of the system-level IP pool endpoints with |
|
It feels bad no matter what I do, which is why this has gone unfixed for so long. Sad. |
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.
@karencfv what do you think about prefixing all of the system-level IP pool endpoints with system_ for consistency? Most other system endpoints do not have this because they don't have silo-scoped analogues that they need to be distinguished from.
Hmmm, how many are there? When we have endpoints to manage things with multi-rack will we prefix them with fleet or something like that? I think I'm leaning to no prefixes. Because then we could argue that all of the other endpoints should have a prefix too like project or cloud and it could get messy? WDYT?
Even though it feels a bit weird like this, I think it'll end up way weirder if we prefix everything. Perhaps the rule can be "If you can apply the same action against different scopes we prefix, if we cannot, no prefix" Thoughts?
| tags = ["system/ip-pools"], | ||
| }] | ||
| async fn ip_pool_view( | ||
| async fn system_ip_pool_view( |
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.
This one should have a description as well no?
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.
Mostly to know how it differs from the other fetch IP pool endpoint, otherwise it's not too obvious?
|
|
||
| /// List IP pools | ||
| /// | ||
| /// List all IP pools regardless of silo links. |
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 think "across all silos" is clearer to me, but I'll leave this one up to you!
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.
Clearest thing IMO would be prefixing all the system ones, but the length would be untenable for some, e.g. system_networking_switch_port_apply_settings. Could potentially work with some shuffling and compressing but not sure it's worth doing.
On balance, what you have is probably the best approach and is only a mild smell. Distinguishing on those that exist on both system and silo or project level seems like a reasonable thing to do and document.
Closes #8226. The
system_inconsistency is weird, but I don't want to putsystem_on all of them. I could be persuaded otherwise, but I think even with the inconsistency this is still an improvement on the highly misleading status quo described in the issue.