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.

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

ComputerSystem.Reset add response data to function return #246

Open
sseekamp opened this issue Apr 7, 2023 · 5 comments
Open

ComputerSystem.Reset add response data to function return #246

sseekamp opened this issue Apr 7, 2023 · 5 comments

Comments

@sseekamp
Copy link

sseekamp commented Apr 7, 2023

Here:

func (computersystem *ComputerSystem) Reset(resetType ResetType) error {
// Make sure the requested reset type is supported by the system
valid := false
if len(computersystem.SupportedResetTypes) > 0 {
for _, allowed := range computersystem.SupportedResetTypes {
if resetType == allowed {
valid = true
break
}
}
} else {
// No allowed values supplied, assume we are OK
valid = true
}
if !valid {
return fmt.Errorf("reset type '%s' is not supported by this service",
resetType)
}
t := struct {
ResetType ResetType
}{ResetType: resetType}
return computersystem.Post(computersystem.resetTarget, t)
}

With many Redfish implementations now returning additional data (task urls specifically) it would be appreciated if the function above (and probably others) also returned the response or select attributes of it to the caller.

@stmcginnis
Copy link
Owner

Thanks, this looks like it would be good to expand on these types of actions. At least in the case of ComputerSystem.Reset, the response could/should look at the response code and if it is 202 handle returning a task:

/redfish/v1/Systems/{ComputerSystemId}/Actions/ComputerSystem.Reset:
  post:
    parameters:
    - description: The value of the Id property of the ComputerSystem resource
      in: path
      name: ComputerSystemId
      required: true
        type: string
    requestBody:
      content:
        application/json:
          schema:
            $ref: http://redfish.dmtf.org/schemas/v1/ComputerSystem.v1_6_0.yaml#/components/schemas/ResetRequestBody
      required: true
    responses:
      '200':
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/RedfishError'
        description: The response contains the results of the Reset action
      '202':
        content:
          application/json:
            schema:
              $ref: http://redfish.dmtf.org/schemas/v1/Task.v1_4_0.yaml#/components/schemas/Task
        description: Accepted; a task has been generated
      '204':
        description: Success, but no response data
      default:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/RedfishError'
        description: Error condition

@sseekamp
Copy link
Author

Excellent!

Before I get in over my head with trying to implement that, would you suggest using a common.Operations object or a Task object or just return the raw URL string with the task link if a 202?

@stmcginnis
Copy link
Owner

I think it should probably return a Task object to stay somewhat consistent with the spec definition. I've tried to avoid returning a raw URL string as much as possible to hide the low level details.

The tricky part of this is going to be that the current Post method only returns an error. Since there are several operations that can return a Task, it probably makes sense to change this to return a tuple of (Task, error).

The big downside of doing that is it will require updating everywhere that a Post call is made to either ignore the Task or use it. That may be a useful exercise though, because I think there are a few calls that are like this where they could return a Task.

Then the Task definition itself should probably move under common since it is a more common construct across both redfish and swordfish.

That's my quick take at least. If you start looking in to this and have a better approach, I'd be very happy to hear it. :)

@sseekamp
Copy link
Author

That was my conclusion as well, but I wasn't sure if its as a good one!

I will see what I can do for a first pass and I'd very much appreciate any feedback you'd give me on my approach.

@sseekamp
Copy link
Author

This is very much a WIP set of changes:
#249

I did specific testing around Tasks and the Reset method, but there were changes in a number of other files that will need more thorough testing if this is a suitable direction to go.

I also made a small change around the service service.Tasks() method not returning data that was unrelated to my changes, but I couldn't test reliably without updating.

Feedback welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants