feat(oem/supermicro): Add Supermicro Manager OEM functions #213
Conversation
|
Awesome!
The way it was invisioned for these OEM extensions it would be a matter of creating the OEM type from the standard type. So something roughly along these lines: manager := c.GetManager()
smManager := supermicro.FromManager(manager)
for _, logs := range smManager.SysLogs() {
// process
}So rather than trying to mirror the whole structure, you just hydrate a new OEM object using the existing Redfish object. A decent example is probably here: https://github.com/stmcginnis/gofish/blob/main/oem/dell/eventservice.go |
|
@stmcginnis If I understand the Redfish Specification correctly, Supermicros Manager OEM extensions are separate resouces and not part of the manager itself, while Dells Eventservice contains the extensions directly. Does this make a difference in how the Supermicro extensions should be accessed from the manager? |
Thanks for the details. That may change a little of how the resulting structs are defined, but that wouldn't change how they are accessed. All OEM data should be accessed by using the standard Redfish objects, then using an OEM implementation to access the |
|
I modified the manager accordingly. |
|
|
||
| type Manager struct { | ||
| redfish.Manager | ||
| Oem ManagerOem |
There was a problem hiding this comment.
It might be a simple interface to just get rid of this Oem field and move the fields of ManagerOem up a level into Manager.
There was a problem hiding this comment.
AFAICT, this is just matching the JSON structure
There was a problem hiding this comment.
Yes, but it's an unnecessary detail for consumers. In the JSON unmarhalling it can pull out the fields and set them as direct properties of the manager struct.
There was a problem hiding this comment.
Ah, I think I know what you mean now. Unexport the whole Oem field?
There was a problem hiding this comment.
Yep. You're already using an OEM object, so it feels a little awkward to have to have OEMObject.OEM.Foo instead of OEMObject.Foo.
There was a problem hiding this comment.
Close, but I think this would be best:
type Manager struct {
redfish.Manager
fanMode string
iKVM string
ipAccessControl string
kcsInterface string
licenseManager string
mouseMode string
ntp string
radius string
smcrakp string
snmp string
smtp string
snooping string
sysLockdown string
syslog string
}
type managerOem struct {
Supermicro struct {
ODataType string `json:"@odata.type"`
FanMode common.Link
IKVM common.Link
IPAccessControl common.Link
KCSInterface common.Link
LicenseManager common.Link
MouseMode common.Link
NTP common.Link
RADIUS common.Link
SMCRAKP common.Link
SNMP common.Link
SMTP common.Link
Snooping common.Link
SysLockdown common.Link
Syslog common.Link
} `json:"Supermicro"`
}
func FromManager(manager *redfish.Manager) (Manager, error) {
var oem managerOem
err := json.Unmarshal(manager.Oem, &oem)
if err != nil {
return Manager{}, fmt.Errorf("can't unmarshal OEM Manager: %v", err)
}
return Manager{
Manager: *manager,
fanMode: oem.Supermicro.FanMode.String(),
iKVM: oem.Supermicro.IKVM.String(),
ipAccessControl: oem.Supermicro.IPAccessControl.String(),
kcsInterface: oem.Supermicro.KCSInterface.String(),
licenseManager: oem.Supermicro.LicenseManager.String(),
mouseMode: oem.Supermicro.MouseMode.String(),
ntp: oem.Supermicro.NTP.String(),
radius: oem.Supermicro.RADIUS.String(),
smcrakp: oem.Supermicro.SMCRAKP.String(),
snmp: oem.Supermicro.SNMP.String(),
smtp: oem.Supermicro.SMTP.String(),
snooping: oem.Supermicro.Snooping.String(),
sysLockdown: oem.Supermicro.SysLockdown.String(),
syslog: oem.Supermicro.Syslog.String(),
}, nil
}
func (manager *Manager) NTP() (*NTP, error) {
return GetNTP(manager.GetClient(), manager.ntp)
}
func (manager *Manager) Syslog() (*Syslog, error) {
return GetSyslog(manager.GetClient(), manager.syslog)
}
func (manager *Manager) IKVM() (*IKVM, error) {
return GetIKVM(manager.GetClient(), manager.iKVM)
}There was a problem hiding this comment.
I can do it like that, but it feels very repetitive to me. Why is the type of, for example Manager.fanMode important, when it's not exported anyway?
There was a problem hiding this comment.
You can omit the extra ones that you are not adding accessor methods for. I just put them in for completeness.
This is more repetitive for the implementation side so it's cleaner for the library consumer.
There was a problem hiding this comment.
In other words, we implement it once here so others never need to think about it.
| type ManagerOem struct { | ||
| Supermicro struct { | ||
| OdataType string `json:"@odata.type"` | ||
| NTP common.Link |
There was a problem hiding this comment.
Then make these fields lower case. The links aren't needed by the user of the library, just the methods being added below to access the objects that those links point to.
So combined with the above, it would just be something like:
smManager, err := supermicro.FromManager(redfishManager)
if err != nil {
// Handle it
}
ntp, err := smManager.NTP()There was a problem hiding this comment.
The fields have to be exported to load them with encoding/json
There was a problem hiding this comment.
You need them when unmarshalling, but not after that. See elsewhere in the library where the unmarshalling takes care of accessing internal details, then stores them as non-public fields that are used in a more user-friendly interface.
This commit includes functions for: * IKVM (Java/HTML5) * NTP (Enabled/Servers/DST) * Syslog (Enabled/Server)
feuerrot
force-pushed
the
mwt_oem_supermicro
branch
from
c02838e
to
b31ba94
Compare
oem/supermicro/ikvm.go
Outdated
| type IKVM struct { | ||
| common.Entity | ||
|
|
||
| OdataType string `json:"@odata.type"` |
There was a problem hiding this comment.
We've standardized on OData (versus Odata) elsewhere. Mind changing these to be consistent?
|
Hi, |
Yes, I'll look into this. |
|
@stmcginnis I responded to all the open review comments. I decided to add another commit to the pull request, since previous review changes also appeared to be extra commits. I would suggest squashing it all together at the end. |
|
|
||
| type Manager struct { | ||
| redfish.Manager | ||
| Oem ManagerOem |
There was a problem hiding this comment.
Yes, but it's an unnecessary detail for consumers. In the JSON unmarhalling it can pull out the fields and set them as direct properties of the manager struct.
|
Sorry, we've had a push to add more Supermicro support, so unfortunately this is now superceded by other work. There's some great stuff here, so feel free to add to or improve the current code in the repo. Thanks a ton for your effort on this! |
|
@stmcginnis No worries, looks like you added everything we needed :) |
This PR includes functions for:
I'd appreciate any hints regarding the
supermicro.Manager-structure and how custom manager extensions should be integrated - while it does work accessing the syslog configuration usingsmm.supermicroManager.Oem.Supermicro.Syslogs()seems to be rather awkward.