stmcginnis / gofish

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

feature: support pass apply time when set BIOS Attr #172

Closed Sn0rt closed 2 years ago

Sn0rt commented 2 years ago

In many cases. I think that if UpdateBiosAttributes function can pass apply time is easy to use .

this case is convert from the DELL bios setting

RACADM set bios.SysProfileSettings.SysProfile PerfPerWattOptimizedOs
RACADM jobqueue create BIOS.Setup.1-1

to set the logger to show the http request of setting as follow

type PrintHttp2OutputWrite struct {
}

func (p *PrintHttp2OutputWrite) Write(b []byte) (n int, err error) {
    return fmt.Printf("%s", string(b))
}

and set the Write to gofish config

        config := gofish.ClientConfig{
...
            DumpWriter: PrintOut,
        }
PATCH /redfish/v1/Systems/System.Embedded.1/Bios/Settings HTTP/1.1
Host: 192.168.156.16
User-Agent: gofish/1.0
Connection: close
Content-Length: 91
Accept: application/json
Content-Type: application/json
Cookie: sessionKey=fa0aeb6a19ca6813deb08b933a0cc5b3
X-Auth-Token: fa0aeb6a19ca6813deb08b933a0cc5b3
Accept-Encoding: gzip

{"@Redfish.SettingsApplyTime":{"ApplyTime":"OnReset"},"Attributes":{"SysProfile":"Custom"}}
HTTP/1.1 202 Accepted
Connection: close
Content-Length: 0
Access-Control-Allow-Origin: *
Cache-Control: no-cache
Content-Type: application/json;odata.metadata=minimal;charset=utf-8
Date: Tue, 10 May 2022 01:23:09 GMT
Location: /redfish/v1/TaskService/Tasks/JID_521457899106
Odata-Version: 4.0
Server: Apache
Strict-Transport-Security: max-age=63072000; includeSubDomains; preload
Www-Authenticate: Basic realm="RedfishService"
X-Frame-Options: DENY

Signed-off-by: Guohao Wang wangguohao.2009@gmail.com

stmcginnis commented 2 years ago

Thanks @Sn0rt - I think this looks great. The only thing I am concerned about is changing the function signature. There may be existing users of the library that we would break by making that change.

In many (most?) cases, the user will not care about specifying apply time. So can you update this to add a new function call that takes the two arguments? Then the existing call can just call out to that new function with common.OnResetApplyTime as a default value.

Sn0rt commented 2 years ago

Thanks @Sn0rt - I think this looks great. The only thing I am concerned about is changing the function signature. There may be existing users of the library that we would break by making that change.

In many (most?) cases, the user will not care about specifying apply time. So can you update this to add a new function call that takes the two arguments? Then the existing call can just call out to that new function with common.OnResetApplyTime as a default value.

no problem . I will to add new func and the prototype of func is func (bios *Bios) UpdateBiosAttributesApplyAt (attrs BiosAttributes, applyTime common.ApplyTime) error

what do you think ?

stmcginnis commented 2 years ago

what do you think ?

Sounds great - thanks!

Sn0rt commented 2 years ago

what do you think ?

Sounds great - thanks!

the commmit has been submitted

stmcginnis commented 2 years ago

Sorry, ended up busy yesterday and couldn't get back to this.

I'd like to not duplicate the code, so still think the one should call the other. But I can clean that up later - this looks good.

Sn0rt commented 2 years ago

thank you