feat(oem/supermicro): Add Supermicro Manager OEM functions

[feuerrot]

This PR includes functions for:

• IKVM (Java/HTML5)
• NTP (Enabled/Servers/DST)
• Syslog (Enabled/Server)

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 using smm.supermicroManager.Oem.Supermicro.Syslogs() seems to be rather awkward.

[stmcginnis]

Awesome!

while it does work accessing the syslog configuration using smm.supermicroManager.Oem.Supermicro.Syslogs() seems to be rather awkward.

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

[feuerrot]

@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?

[stmcginnis]

@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 oem parts to handle vendor specific (i.e. non-standard) paths.

[feuerrot]

I modified the manager accordingly.

[stmcginnis]

Looks great! I've added some suggestions, so I'll hold off on merging until you've had a chance to take a look and respond.

[stmcginnis]

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.

[mxey]

AFAICT, this is just matching the JSON structure

[stmcginnis]

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.

[mxey]

Ah, I think I know what you mean now. Unexport the whole Oem field?

[stmcginnis]

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.

[mxey]

We cannot do both. The JSON-based fields have to remain exported for the unmarshalling, so if we inlined ManagerOem into Manager, all the fields would be visible there. If we leave the extra indirection as-is, we can unexport the Oem field, and the JSON fields would all be unexported.

I think we should do it like this:

type Manager struct {
	redfish.Manager
	supermicro supermicro
}

type supermicro struct {
	ODataType string `json:"@odata.type"`
	NTP       common.Link
	Syslog    common.Link
	IKVM      common.Link
}

func FromManager(manager *redfish.Manager) (Manager, error) {
	type oemFields struct {
		Supermicro supermicro `json:"Supermicro"`
	}
	var oem oemFields
	err := json.Unmarshal(manager.Oem, &oem)
	if err != nil {
		return Manager{}, fmt.Errorf("can't unmarshal OEM Manager: %v", err)
	}

	return Manager{
		Manager:    *manager,
		supermicro: oem.Supermicro,
	}, nil
}

[stmcginnis]

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()

[mxey]

The fields have to be exported to load them with encoding/json

[stmcginnis]

Sorry for taking ridiculously long to get back to this. There are a couple comments still not addressed. If you can make a few updates, this looks really good and we can get it merged.

Thanks for working on this!

[stmcginnis]

We've standardized on OData (versus Odata) elsewhere. Mind changing these to be consistent?

[mxey]

I changed it.

[feuerrot]

Hi,
sorry for my late reply: I don't have access to the Supermicro hardware or the fork anymore, as I switched jobs last year. Maybe @mxey could add the missing changes to this PR?

[mxey]

Maybe @mxey could add the missing changes to this PR?

Yes, I'll look into this.

[mxey]

@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.

[stmcginnis]

Thanks, I'll take a closer look soon!

[stmcginnis]

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.