-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: workflow support #68
base: master
Are you sure you want to change the base?
Conversation
#43 This will add the Workflow support |
@Cliftonz request for review & merge |
@mahendraHegde Can you look at this? |
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.
@srikanth597 plz test all methods in your local against live server using your novu account, some of these methods are breaking
return &resp, nil | ||
} | ||
|
||
func (w *WorkflowService) Create(ctx context.Context, request WorkflowData) (*WorkflowData, error) { |
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.
its ideal to create WorkFlowCreatePayload
in model and use it here, otherwise the sdk method signature would confuse the users, asWorkflowData
is a superset of the create payload
return &resp, nil | ||
} | ||
|
||
func (w *WorkflowService) Update(ctx context.Context, key string, request WorkflowData) (*WorkflowData, error) { |
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.
same as create
Data bool `json:"data"` | ||
} | ||
|
||
type WorkflowUpdateStatus struct { |
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.
type WorkflowUpdateStatus struct { | |
type WorkflowUpdateStatusPayload struct { |
PreferenceSettings Channel `json:"preferenceSettings"` | ||
Critical bool `json:"critical"` | ||
Tags []string `json:"tags"` | ||
Steps []interface{} `json:"steps"` |
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.
any reason for keeping all these as interface, its ideal to have them as concrete structs gives better DX for the sdk users
BlueprintId string `json:"blueprintId,omitempty"` | ||
} | ||
type WorkflowGetResponse struct { | ||
Data WorkflowData `json:"data"` |
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.
Data WorkflowData `json:"data"` | |
Data []WorkflowData `json:"data"` |
swagger seems to have wrong response structure list returns array of workflows, it was failing to unmarshal when i treied to test locally, can you test all workflow methods once, swagger doesnt seem 100% accurate
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.
@Cliftonz swagger needs to be updated here.
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.
Can you put in an issue on the main repo?
return &resp, nil | ||
} | ||
|
||
func (w *WorkflowService) Get(ctx context.Context, key string) (*WorkflowData, error) { |
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.
response type is not WorkflowData
itself its wrapped inside data
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.
same for update and updateStatus
WorkflowIntegrationStatus interface{} `json:"workflowIntegrationStatus,omitempty"` | ||
BlueprintId string `json:"blueprintId,omitempty"` | ||
} | ||
type WorkflowGetResponse struct { |
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.
type WorkflowGetResponse struct { | |
type WorkflowListResponse struct { |
return &resp, nil | ||
} | ||
|
||
func (w *WorkflowService) UpdateStatus(ctx context.Context, key string) (*WorkflowData, error) { |
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.
its not accepting status? which is required in the api, so this is failing also
No description provided.