Skip to content

Update to omicron 5cfd735#284

Merged
karencfv merged 1 commit intooxidecomputer:mainfrom
karencfv:update
Apr 23, 2025
Merged

Update to omicron 5cfd735#284
karencfv merged 1 commit intooxidecomputer:mainfrom
karencfv:update

Conversation

@karencfv
Copy link
Collaborator

No description provided.

@karencfv karencfv requested a review from sudomateo April 22, 2025 00:42
Comment on lines +763 to +765
if len(keys) == 1 && p.Value.Enum == nil {
enumFieldName = propertyName
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code handles the case for https://github.com/oxidecomputer/oxide.go/pull/284/files#diff-841a2b1b5f072b633551cac9c34c9732a722b415e297a1530b92d5988962062eR3728-R3750 Without handling, the result was that they all were named InterfaceNum because they aren't being explicitly marked as an enum, but used as one, and OpenAPI still sees this as a valid spec

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me!

Copy link
Collaborator

@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 haven't used the affected upstream API routes but the changes here look good to me.

// Required fields:
// - IEEE802
type NetworkAddressIeee802 struct {
IEEE802 []string `json:"i_e_e_e802,omitempty" yaml:"i_e_e_e802,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The i_e_e_e802 JSON field read weird to me but that's how it is in the OpenAPI specification on omicron.

Comment on lines +763 to +765
if len(keys) == 1 && p.Value.Enum == nil {
enumFieldName = propertyName
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me!

@sudomateo
Copy link
Collaborator

Just FYI the commit and PR title typo Upodate. Should be Update.

@karencfv karencfv changed the title Upodate to omicron 5cfd735 Update to omicron 5cfd735 Apr 23, 2025
@karencfv karencfv merged commit 65b1d0f into oxidecomputer:main Apr 23, 2025
2 checks passed
@karencfv karencfv deleted the update branch April 23, 2025 01:14
@karencfv
Copy link
Collaborator Author

Just FYI the commit and PR title typo Upodate. Should be Update.

lol whoops

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