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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: workflow support #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

srikanth597
Copy link
Contributor

No description provided.

@srikanth597
Copy link
Contributor Author

#43 This will add the Workflow support

@srikanth597
Copy link
Contributor Author

@Cliftonz request for review & merge

@Cliftonz
Copy link
Contributor

Cliftonz commented Nov 1, 2023

@mahendraHegde Can you look at this?

Copy link
Contributor

@mahendraHegde mahendraHegde left a 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) {
Copy link
Contributor

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) {
Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type WorkflowUpdateStatus struct {
type WorkflowUpdateStatusPayload struct {

PreferenceSettings Channel `json:"preferenceSettings"`
Critical bool `json:"critical"`
Tags []string `json:"tags"`
Steps []interface{} `json:"steps"`
Copy link
Contributor

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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor

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.

Copy link
Contributor

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) {
Copy link
Contributor

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

Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type WorkflowGetResponse struct {
type WorkflowListResponse struct {

return &resp, nil
}

func (w *WorkflowService) UpdateStatus(ctx context.Context, key string) (*WorkflowData, error) {
Copy link
Contributor

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants