Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
May 12, 2022
Merged

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

merged 1 commit into from
May 12, 2022

Conversation

Sn0rt
Copy link
Contributor

@Sn0rt Sn0rt commented May 10, 2022 •
edited
Loading

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 [email protected]

@stmcginnis
Copy link
Owner

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
Copy link
Contributor Author

Sn0rt commented May 11, 2022 •
edited
Loading

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
Copy link
Owner

what do you think ?

Sounds great - thanks!

@Sn0rt
Copy link
Contributor Author

Sn0rt commented May 12, 2022

what do you think ?

Sounds great - thanks!

the commmit has been submitted

@stmcginnis
Copy link
Owner

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.

@stmcginnis stmcginnis merged commit 07ce3e3 into stmcginnis:main May 12, 2022
@Sn0rt Sn0rt deleted the support-set-apply-time branch May 13, 2022 02:22
@Sn0rt
Copy link
Contributor Author

Sn0rt commented May 13, 2022

thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stmcginnis stmcginnis stmcginnis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Sn0rt @stmcginnis