Skip to content

[external api] Set a default empty array for VpcFirewallRuleUpdate #8175

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karencfv
Copy link
Contributor

In our attempt to make the Go SDK generator less hacky oxidecomputer/oxide.go#283, we had the unexpected side effect of breaking our VpcFirewallRuleUpdateParams struct https://github.com/oxidecomputer/oxide.go/pull/283/files#diff-841a2b1b5f072b633551cac9c34c9732a722b415e297a1530b92d5988962062eR6810-R6811 due to Go's fun way of handling empty values. Using this API no longer allows us to delete the entirety of the firewall rules.

This commit sets an empty array as the default for rules following the pattern that other APIs use.
https://github.com/oxidecomputer/omicron/blob/main/nexus/types/src/external_api/params.rs#L1163-L1177

cc @sudomateo

@karencfv karencfv requested a review from david-crespo May 16, 2025 02:05
@karencfv karencfv changed the title [api] Set a default empty array for VpcFirewallRuleUpdate [external api] Set a default empty array for VpcFirewallRuleUpdate May 16, 2025
Copy link
Contributor

@sudomateo sudomateo left a comment

Choose a reason for hiding this comment

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

I'm cool with this change to be consistent with other APIs but I would have assumed even without this change setting rules to [] in the request body would have worked. Did the previous configuration enforce a non-empty array?

Oh, I think I see it now. The Go SDK has omitempty on the rules attribute so setting [] serializes to null. The rules attribute isn't nullable in omicron so the omitempty will remain in the Go SDK. Now sending null in the request body will allow omicron to default rules to []. Do I have that right?

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