-
Notifications
You must be signed in to change notification settings - Fork 47
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
Added workflow response object to workflow creation call #409
Conversation
Replace workflowId in request/response and returned a workflow object.
exchanges.yml
Outdated
workflowId: | ||
type: string | ||
description: The URL that uniquely identifies the created workflow. | ||
workflowData: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think just the workflow config
should be returned here and it looks like this is something a little different with some additional metadata is here. Certainly an implementation could do more, but I think we should only say that the workflow config is returned (i.e,. the same thing you'd get if you did a GET on the workflow ID).
workflowData: | |
config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of what is returned by the GET workflowId as you are suggesting with the addition of the workflowId itself under the 'id' property as mentioned in the original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see we have a couple of mistakes in what's in the spec now then. There should be no workflowData
property when creating a workflow, rather, the properties here:
Lines 190 to 222 in 5efdafd
properties: | |
steps: | |
type: object | |
description: One or more steps required to complete an exchange on the workflow. Passing the steps object is REQUIRED. | |
properties: | |
stepName: | |
type: object | |
description: The name of the step. | |
properties: | |
step: | |
$ref: "#/components/schemas/WorkflowStep" | |
initialStep: | |
type: string | |
description: The step from the above set that the exchange starts on. Passing intialStep is REQUIRED. | |
controller: | |
type: string | |
description: The controller of the instance. Passing controller is OPTIONAL. | |
authorization: | |
type: string | |
description: Authorization scheme information (e.g., OAuth2 configuration). Passing authorization is OPTIONAL. | |
credentialTemplates: | |
type: array | |
description: One or more VC templates for issuance. Passing credentialTemplates is OPTIONAL. | |
items: | |
type: object | |
properties: | |
type: | |
type: string | |
template: | |
type: string | |
id: | |
type: string | |
description: The ID that will be used for the created workflow. Passing an ID is OPTIONAL. |
Should just be the main top-level properties in the payload, this is the "config" for the workflow.
I also see that the GET workflowId
section is inconsistent and adds that extra stepInformation
bit -- which we should remove (collapse it down so its nested properties are moved up a level so it matches what should be in the above payload for creating the workflow).
exchanges.yml
Outdated
type: object | ||
description: Information about the steps required for the workflow. Returning stepInformation is REQUIRED. | ||
properties: | ||
exchanges: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a potentially huge number of exchanges for a very popular workflow -- and it isn't part of the config, so we shouldn't do this as a requirement, though some implementation might return this kind of data if that is desirable/not problematic.
exchanges.yml
Outdated
stepInformation: | ||
type: object | ||
description: Information about the steps required for the workflow. Returning stepInformation is REQUIRED. | ||
properties: | ||
exchanges: | ||
type: array | ||
description: The identifiers of the current exchanges associated with the workflow instance. | ||
items: | ||
type: string | ||
steps: | ||
type: object | ||
description: One or more steps required to complete an exchange on the workflow. | ||
properties: | ||
stepName: | ||
type: object | ||
description: The name of the step. | ||
properties: | ||
step: | ||
$ref: "#/components/schemas/WorkflowStep" | ||
initialStep: | ||
type: string | ||
description: The step from the above set that the exchange starts on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the stepInformation
bit should be collapsed to what's in the config, i.e., I think steps
and initialStep
are nested here when they shouldn't be. See also note on "current exchanges".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of the config of an existing workflow returned by the get workflowId
route, could you provide an example of what the config should look like if this is not it?
Co-authored-by: Ted Thibodeau Jr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make the required changes if I can get some examples provided.
exchanges.yml
Outdated
stepInformation: | ||
type: object | ||
description: Information about the steps required for the workflow. Returning stepInformation is REQUIRED. | ||
properties: | ||
exchanges: | ||
type: array | ||
description: The identifiers of the current exchanges associated with the workflow instance. | ||
items: | ||
type: string | ||
steps: | ||
type: object | ||
description: One or more steps required to complete an exchange on the workflow. | ||
properties: | ||
stepName: | ||
type: object | ||
description: The name of the step. | ||
properties: | ||
step: | ||
$ref: "#/components/schemas/WorkflowStep" | ||
initialStep: | ||
type: string | ||
description: The step from the above set that the exchange starts on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of the config of an existing workflow returned by the get workflowId
route, could you provide an example of what the config should look like if this is not it?
exchanges.yml
Outdated
id: | ||
type: string | ||
description: The URL that uniquely identifies the created workflow. | ||
stepInformation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the call: Delete stepInformation
exchanges.yml
Outdated
steps: | ||
type: object | ||
description: One or more steps required to complete an exchange on the workflow. | ||
properties: | ||
stepName: | ||
type: object | ||
description: The name of the step. | ||
properties: | ||
step: | ||
$ref: "#/components/schemas/WorkflowStep" | ||
initialStep: | ||
type: string | ||
description: The step from the above set that the exchange starts on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move steps
and initialStep
up/left one level.
exchanges.yml
Outdated
CreateWorkflowResponse: | ||
type: object | ||
additionalProperties: false | ||
description: Object containing information about a created workflow. | ||
description: Response containing the created workflowData Object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change workflowData
-> config
.
Same changes with workflowX
-> X
(in general).
@@ -183,7 +183,7 @@ components: | |||
additionalProperties: false | |||
description: Object containing information for creating a workflow. | |||
properties: | |||
workflowData: | |||
config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a workflow, the config is the payload, it is not nested under config
. I think we should have a WorkflowConfig object that can be referenced and used here as the direct payload and in other places where a config
property is used to express it along with other side-by-side properties (like in the response / potentially other places) this object type can just be referenced -- rather than duplicating its rather long schema description.
@@ -217,49 +217,81 @@ components: | |||
type: string | |||
template: | |||
type: string | |||
workflowId: | |||
id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
can be optional within the WorkflowConfig
object when creating a workflow, indicating either a preferred id
(which some servers may honor, others may reject).
exchanges: | ||
type: array | ||
description: The identifiers of the current exchanges associated with the workflow instance. | ||
items: | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't expect this (exchanges
) to ever be part of a WorkflowConfig
-- and it may be very large in size anyway.
GetWorkflowResponse: | ||
type: object | ||
additionalProperties: false | ||
description: Object containing information about a workflow. | ||
properties: | ||
workflowData: | ||
config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this endpoint to return the WorkflowConfig
directly as the response payload here.
This PR was merged presuming that a WorkflowConfig object will be created to remove duplication of that object. |
Addresses #393
Replace
workflowId
in request/response and returned a workflow object.