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

Add WithContext functions to executor #161

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Add WithContext functions to executor #161

merged 3 commits into from
Nov 7, 2024

Conversation

jmigueprieto
Copy link
Contributor

@jmigueprieto jmigueprieto commented Nov 7, 2024 •
edited
Loading

Changes in this PR

  1. Added WithContext functions to executor e.g.: StartWorkflowWithContext for managing cancellations, timeouts, and deadlines.
  2. Existing functions delegate calls to those new functions using context.Background().
  3. HttpRequester uses http.NewRequestWithContext.
  4. Cleanup + small fixes.
func (e *WorkflowExecutor) StartWorkflow(startWorkflowRequest *model.StartWorkflowRequest) (workflowId string, err error) {
	return e.StartWorkflowWithContext(context.Background(), startWorkflowRequest)
}

func (e *WorkflowExecutor) StartWorkflowWithContext(ctx context.Context, startWorkflowRequest *model.StartWorkflowRequest) (workflowId string, err error) {
	if err := ctx.Err(); err != nil {
		return "", err
	}

	id, _, err := e.workflowClient.StartWorkflowWithRequest(
		ctx,
		*startWorkflowRequest,
	)
	if err != nil {
		return "", err
	}
	return id, nil
}

@jmigueprieto jmigueprieto self-assigned this Nov 7, 2024
@jmigueprieto jmigueprieto force-pushed the feature/with-context-executor-fns branch from fe771ec to e1c0cca Compare November 7, 2024 16:36
@jmigueprieto jmigueprieto changed the title (WIP) Adding WithContext functions to executor (WIP) Add WithContext functions to executor Nov 7, 2024
@jmigueprieto
- Executor functions delegate calls to their WithContext pair.
6f17031 
- These new `WithContext` functions check the context and pass it to the client instead of just using the root context.
@jmigueprieto jmigueprieto force-pushed the feature/with-context-executor-fns branch from e1c0cca to 6f17031 Compare November 7, 2024 19:17
@jmigueprieto jmigueprieto changed the title (WIP) Add WithContext functions to executor Add WithContext functions to executor Nov 7, 2024
@jmigueprieto jmigueprieto requested review from v1r3n, c4lm, codehackerr and a team November 7, 2024 19:22
jmigueprieto
jmigueprieto commented Nov 7, 2024
View reviewed changes
sdk/model/rbac/create_or_update_application_request.go
@@ -11,6 +11,5 @@ package rbac

type CreateOrUpdateApplicationRequest struct {
// Application's name e.g.: Payment Processors
Name string `json:"name"`
Description string `json"description"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no description in CreateOrUpdateApplicationRequest

@jmigueprieto jmigueprieto marked this pull request as ready for review November 7, 2024 19:23
jmigueprieto
jmigueprieto commented Nov 7, 2024
View reviewed changes
sdk/authentication/token_helper.go
@@ -234,9 +234,7 @@ func contains(haystack []string, needle string) bool {
}

func setBody(body interface{}, contentType string) (bodyBuf *bytes.Buffer, err error) {
if bodyBuf == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unnecessary check

jmigueprieto
jmigueprieto commented Nov 7, 2024
View reviewed changes
sdk/authentication/token_helper.go
@@ -248,7 +246,7 @@ func setBody(body interface{}, contentType string) (bodyBuf *bytes.Buffer, err e
} else if jsonCheck.MatchString(contentType) {
err = json.NewEncoder(bodyBuf).Encode(body)
} else if xmlCheck.MatchString(contentType) {
xml.NewEncoder(bodyBuf).Encode(body)
err = xml.NewEncoder(bodyBuf).Encode(body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missing error handling but I don't think this is being used

jmigueprieto
jmigueprieto commented Nov 7, 2024
View reviewed changes
sdk/client/http_requester.go
if body != nil {
localVarRequest, err = http.NewRequest(method, url.String(), body)
localVarRequest, err = http.NewRequestWithContext(ctx, method, url.String(), body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

context was propagated but not used at all before these changes

jmigueprieto
jmigueprieto commented Nov 7, 2024
View reviewed changes
test/integration_tests/executor_test.go
// cancel straightaway on purpose
cancel()

_, err := executor.StartWorkflowWithContext(ctx, &model.StartWorkflowRequest{})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example test that shows that StartWorkflowWithContext throws an error if called with a cancelled context.

@jmigueprieto jmigueprieto merged commit 3891d83 into main Nov 7, 2024
3 checks passed
@jmigueprieto jmigueprieto deleted the feature/with-context-executor-fns branch November 7, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c4lm c4lm c4lm approved these changes

@v1r3n v1r3n Awaiting requested review from v1r3n

@codehackerr codehackerr Awaiting requested review from codehackerr

Assignees

@jmigueprieto jmigueprieto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmigueprieto @c4lm