stmcginnis / gofish

Gofish is a Golang client library for DMTF Redfish and SNIA Swordfish interaction.
BSD 3-Clause "New" or "Revised" License
224 stars 117 forks source link

updated virtualmedia according version v1.3.2 #120

Closed mikeletux closed 3 years ago

mikeletux commented 3 years ago

Hi Sean,

I'm sending this PR to update virtualMedia according redfish schema v1.3.2.

Now apart from Image, Inserted and WriteProtected we can pass more params:

type temp struct {
        Image                string
        Inserted             bool
        Password             string `json:",omitempty"`
        TransferMethod       string `json:",omitempty"`
        TransferProtocolType string `json:",omitempty"`
        UserName             string `json:",omitempty"`
        WriteProtected       bool
    }

Also changed the signature from InsertMedia function to accomodate new inputs:

func (virtualmedia *VirtualMedia) InsertMedia(image string,
    inserted bool,
    password string,
    transferMethod string,
    transferProtocolType string,
    userName string,
    writeProtected bool) error

Last, I've noticed that some implementations fail when it come to use the EjectMedia() function, since nil is passed as payload, and so no body in included. I've solved that by passing an empty struct so an empty body is sent:

virtualmedia.Client.Post(virtualmedia.ejectMediaTarget, struct{}{})

Also updated the unit tests and I've done some success integration tests as well.

Let me know! Best regards. /Miguel

stmcginnis commented 3 years ago

This looks great @mikeletux. One issue though - looks like unit tests are failing with the EjectMedia change. For some reason the GitHub Action isn't running (I guess I better investigate that) but running make test locally should show the failure:

    virtualmedia_test.go:154: Unexpected EjectMedia payload: map[]
--- FAIL: TestVirtualMediaEject (0.00s)
stmcginnis commented 3 years ago

Just a thought, interested in your opinion...

I wonder if it would be worth keeping backwards compatibility and adding a new InsertMedia(args InsertMediaConfig) call that would take a struct with relevant data. Then make your type temp struct {... into a public struct, something like type InsertMediaConfig struct{ that could be populated as needed.

Probably not likely that this call will keep adding more args, but that could give a way to keep things stable if/when new things are added to the interface.

Just something to think about.

BTW, check job is fixed, so next push should trigger jobs.

mikeletux commented 3 years ago

Hey Sean,

Yeah, I agree. Let's create a public struct with all that data and keep the old function for backward compatibility. Also it will be much cleaner to use an struct to pass all that data rather having a function signature that long..

Let me work in that piece of code and I'll let you know one it's pushed.

Thanks! /Miguel