-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Unmarshal(cfg string) to VendorConfigManager #17
Conversation
dd3030c
to
cfd9f1a
Compare
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.
Looks good in general, although could we have some tests and fixtures added so these are maintainable and can be grokked by another person when necessary
@@ -114,13 +125,208 @@ func (cm *supermicroVendorConfig) Marshal() (string, error) { | |||
} | |||
} | |||
|
|||
func (cm *supermicroVendorConfig) Unmarshal(cfgData string) (err error) { |
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 you include test cases and some fixture data for these
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'd like to produce a future PR with test cases for all the config stuff and not block this one for now.
return | ||
} | ||
|
||
func normalizeSetting(s *supermicroBiosCfgSetting) (k, v string, err error) { |
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.
Test cases for this method would help maintain this over time
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.
Same comment re a future PR.
} | ||
|
||
func (cm *supermicroVendorConfig) EnableSRIOV() { | ||
cm.Raw("SR-IOV Support", "Enabled", []string{"Advanced", "PCIe/PCI/PnP Configuration"}) | ||
func (cm *supermicroVendorConfig) BootOrder(mode string) error { |
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.
test case here would help
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.
And here
…aining generic funcs
168c36c
to
bac336d
Compare
Unmarshal(cfgData string) (err error) | ||
StandardConfig() (biosConfig map[string]string, err error) | ||
|
||
BootMode(mode string) error |
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.
Would recommend smaller interfaces, so not all vendor implementations are forced to implement all of these
What does this PR implement/change/remove?
Adds a function to each VendorConfigManager with the signature Unmarshal(cfg string) that takes a vendor-specific raw config file (as a string) and parses that content into the vendor specific configuration structs.
Additionally, this commit also provides functions for config/supermicro that convert a vendor specific config file into a generic map[string]string normalized across vendors.
The HW vendor this change applies to (if applicable)
Supermicro primarily.
Description for changelog/release notes