Support Storage Controllers in new (v1.13.0) versions of redfish

[rounak-adhikary]

Context

Older versions of redfish (eg https://redfish.dmtf.org/schemas/v1/Storage.v1_1_0.json) returned storage controllers as an array embedded within the parent storage object. The current implementation of this repo is in line with that (ref: https://github.com/stmcginnis/gofish/blob/99eeed0e783a6e12f5fe19f4d74f549b6d754f1b/redfish/storage.go#L53).

But new versions of redfish (eg https://redfish.dmtf.org/schemas/v1/Storage.v1_13_0.json) return the Storage Controller collection odata ID, which is used to get Storage Controllers (in line with drives, enclosures and volumes retrieval procedure, eg. https://github.com/stmcginnis/gofish/blob/99eeed0e783a6e12f5fe19f4d74f549b6d754f1b/redfish/storage.go#L199).

Feature Request

Support the new version method of getting Storage Controller

Discussion

I can create the PR for this, but I have one question: How to handle the old and new storage controller schemas simultaneously? The issue is that the old and new storage controller objects have different fields and the struct named "StorageController" can represent only 1 of them. There are 3 options:

1. Remove the old storage controller schema support completely (will break older schema compatibility).
2. Have "StorageController" represent the newer object and a new struct called "EmbeddedStorageController" represent the older one. Will impact backward compatibility just a bit, but the newer schema gets the "main" struct.
3. Have "StorageController" represent the older object and a new struct called "ExtendedStorageController" represent the newer one. Will be fully backward compatible, but the "main" struct will be representing a deprecated object.

Please let me know your thoughts. Thanks.

[stmcginnis]

Thanks for raising this and looking in to it!

In general, it's been an unwritten goal of the project to try to make the progression from older schema versions to newer versions as painless as possible for the consumer, as much as that's actually possible. It's really unfortunate there are so many breaking changes with the way the new controllers are implemented.

Part of the hope with that is to try not to break existing consumers of the library. So I'd love to just go with 2 and focus on the new implementations, but there is a high risk of breaking someone by doing that. I think that means that option 3 is the least disruptive, but would love to hear any other opinions.

It does look like the properties of the older StorageController all exist in the newer definition. So I think it might be possible to do something like:

type ExtendedStorageController struct {
    StorageController
    Certificates String
    ....
}

Then if someone is expecting the older struct, they could still treat it as one if all they need is one of the existing properties.

That would be awesome (and very welcome!) if you could propose a PR. Thanks again for looking into this!

[rounak-adhikary]

@stmcginnis I love the idea. Let's go with option 3 and implement ExtendedStorageController with an embedded StorageController. Thanks for your suggestions!

Will get a PR up soon.

[stmcginnis]

Looking at the spec, the new linked property name is actually Controllers now. That was picked up in the recent refresh to the 2023.3 bundle. I do see I missed noting the deprecation of the older method, so I will quick add that and close this out.

Thanks for raising this!

[rounak-adhikary]

Thanks @stmcginnis for making the changes!