Skip to content
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

Merged
merged 18 commits into from
Jul 23, 2024
Merged

Conversation

splaspood
Copy link
Collaborator

@splaspood splaspood commented Jun 5, 2024

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

* Add Unmarshal(cfg string) to VendorConfigManager
* Add supermicro configuration normalization functions to config/supermicro

Copy link
Member

@joelrebel joelrebel left a 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

config/asrockrack.go Outdated Show resolved Hide resolved
config/dell.go Outdated Show resolved Hide resolved
config/dell.go Outdated Show resolved Hide resolved
config/asrockrack.go Outdated Show resolved Hide resolved
config/supermicro.go Outdated Show resolved Hide resolved
@@ -114,13 +125,208 @@ func (cm *supermicroVendorConfig) Marshal() (string, error) {
}
}

func (cm *supermicroVendorConfig) Unmarshal(cfgData string) (err error) {
Copy link
Member

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

Copy link
Collaborator Author

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) {
Copy link
Member

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

Copy link
Collaborator Author

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.

config/supermicro.go Show resolved Hide resolved
config/supermicro.go Show resolved Hide resolved
}

func (cm *supermicroVendorConfig) EnableSRIOV() {
cm.Raw("SR-IOV Support", "Enabled", []string{"Advanced", "PCIe/PCI/PnP Configuration"})
func (cm *supermicroVendorConfig) BootOrder(mode string) error {
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And here

Unmarshal(cfgData string) (err error)
StandardConfig() (biosConfig map[string]string, err error)

BootMode(mode string) error
Copy link
Member

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

@splaspood splaspood merged commit 8256ba7 into main Jul 23, 2024
3 checks passed
@splaspood splaspood deleted the vcm-unmarshal-0 branch July 23, 2024 13:58
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