IWF-70: allow headers to be set in Interpreter Config #424

samuel27m · 2024-09-23T17:45:38Z

Closes #436

samuel27m · 2024-09-23T17:50:25Z

integ/util.go

@@ -67,6 +67,7 @@ type IwfServiceTestConfig struct {
 	MemoEncryption                   bool
 	DisableFailAtMemoIncompatibility bool // default to false so that we will fail at test
 	OptimizedVersioning              *bool
+	DefaultHeaders                   map[string]string


I saw that the previous implementation had integration tests that would check for the presence of the headers in every call, am I seeing that right? https://github.com/indeedeng/iwf/pull/356/files#diff-c3752f9aebfa9a670e62a5c0cc34c36bbfbfcbe849288ece0973806a34eac156R30-R33

I wanted to do something more flexible, where we would have a specific integration test for checking the headers passed in the configuration are being used, rather than having the route check it every time a call was made.

I do need some of your guidance here, I can't figure out how to do the integration tests with this approach.

The headers will be used when making calls for stateAPI. So for a specific integ tests, you will still need to build out a router. But it will be cleaner to have that specific router, is that what you meant?

That's what I'm thinking yes. Do you think that would be overkill for testing such a small feature?

I think it’s a good idea. I have been lazy to do the right thing and the tests have become messy here

I have created the tests now, I struggled a bit navigating through the existing ones trying to use one as a template. I'm sure the tests I created can be better. We can have a chat tomorrow about them and you can point me where I've gone wrong! Thanks in advance! 🙌

Sounds good. I went through it and left you some comments already. and we can go through them later

samuel27m · 2024-09-23T17:51:26Z

@longquanzheng If you have some time tomorrow, let's chat tomorrow about this PR. I do need some guidance on the integration tests in the comment above.

longquanzheng · 2024-09-25T16:39:16Z

integ/workflow/headers/routers.go

+const (
+	WorkflowType = "basic"
+	State1       = "S1"
+	State2       = "S2"


Nit: We can only have one state for this test

longquanzheng · 2024-09-25T16:39:26Z

integ/workflow/headers/routers.go

+)
+
+const (
+	WorkflowType = "basic"


Nit: maybe "header"?

longquanzheng · 2024-09-25T16:39:53Z

integ/workflow/headers/routers.go

+	if context.GetAttempt() <= 0 || context.GetFirstAttemptTimestamp() <= 0 {
+		panic("attempt and firstAttemptTimestamp should be greater than zero")
+	}


you don't have to check this

longquanzheng · 2024-09-25T16:42:34Z

integ/headers_test.go

+	// start a workflow
+	sharedConfig := env.GetSharedConfig()
+	apiClient := iwfidl.NewAPIClient(&iwfidl.Configuration{
+		DefaultHeader: sharedConfig.Interpreter.InterpreterActivityConfig.DefaultHeader,


I don't think we need this?

longquanzheng · 2024-09-25T16:43:00Z

integ/headers_test.go

+	req := apiClient.DefaultApi.ApiV1WorkflowStartPost(context.Background())
+	startReq := iwfidl.WorkflowStartRequest{
+		WorkflowId:             wfId,
+		IwfWorkflowType:        basic.WorkflowType,


We should start the header workflow here

longquanzheng · 2024-09-25T16:43:20Z

integ/headers_test.go

+		IwfWorkflowType:        basic.WorkflowType,
+		WorkflowTimeoutSeconds: 100,
+		IwfWorkerUrl:           "http://localhost:" + testWorkflowServerPort,
+		StartStateId:           ptr.Any(basic.State1),


the starting state should be the header.State1

longquanzheng · 2024-09-25T16:43:56Z

integ/headers_test.go

+			WorkflowIDReusePolicy:  ptr.Any(iwfidl.REJECT_DUPLICATE),
+			// TODO: need more work to write integ test for cron
+			// manual testing for now by uncomment the following line
+			// CronSchedule:           iwfidl.PtrString("* * * * *"),
+			RetryPolicy: &iwfidl.WorkflowRetryPolicy{
+				InitialIntervalSeconds: iwfidl.PtrInt32(11),
+				BackoffCoefficient:     iwfidl.PtrFloat32(11),
+				MaximumAttempts:        iwfidl.PtrInt32(11),
+				MaximumIntervalSeconds: iwfidl.PtrInt32(11),
+			},
+		},
+		StateOptions: &iwfidl.WorkflowStateOptions{
+			StartApiTimeoutSeconds:  iwfidl.PtrInt32(12),
+			DecideApiTimeoutSeconds: iwfidl.PtrInt32(13),
+			StartApiRetryPolicy: &iwfidl.RetryPolicy{
+				InitialIntervalSeconds: iwfidl.PtrInt32(12),
+				BackoffCoefficient:     iwfidl.PtrFloat32(12),
+				MaximumAttempts:        iwfidl.PtrInt32(12),
+				MaximumIntervalSeconds: iwfidl.PtrInt32(12),
+			},
+			DecideApiRetryPolicy: &iwfidl.RetryPolicy{
+				InitialIntervalSeconds: iwfidl.PtrInt32(13),
+				BackoffCoefficient:     iwfidl.PtrFloat32(13),
+				MaximumAttempts:        iwfidl.PtrInt32(13),
+				MaximumIntervalSeconds: iwfidl.PtrInt32(13),
+			},
+		},


we can skip all these

longquanzheng · 2024-09-25T16:44:40Z

integ/headers_test.go

+	resp2, httpResp, err := req2.WorkflowGetRequest(iwfidl.WorkflowGetRequest{
+		WorkflowId: wfId,
+	}).Execute()
+	panicAtHttpError(err, httpResp)
+
+	// use a wrong workflowId to test the error case
+	_, _, err = req2.WorkflowGetRequest(iwfidl.WorkflowGetRequest{
+		WorkflowId: "a wrong workflowId",
+	}).Execute()
+	apiErr, ok = err.(*iwfidl.GenericOpenAPIError)
+	if !ok {
+		log.Fatalf("Should fail to invoke get api %v", err)
+	}
+	errResp, ok = apiErr.Model().(iwfidl.ErrorResponse)
+	if !ok {
+		log.Fatalf("should be error response")
+	}
+	assertions.Equal(errResp.GetSubStatus(), iwfidl.WORKFLOW_NOT_EXISTS_SUB_STATUS)


we can skip these. So that the test is focusing on the header stuff only

samuel27m · 2024-09-27T14:45:22Z

Thanks for all the test feedback @longquanzheng! This should be good to review now, let me know if there's something that you think I could simplify further! Thanks in advance! 🙌

longquanzheng · 2024-09-27T15:18:53Z

config/config.go

@@ -68,6 +68,7 @@ type (
 		// default is http://localhost:ApiConfig.Port
 		ApiServiceAddress                  string                              `json:"serviceAddress"`
 		DumpWorkflowInternalActivityConfig *DumpWorkflowInternalActivityConfig `json:"dumpWorkflowInternalActivityConfig"`
+		DefaultHeader                      map[string]string                   `json:"defaultHeader"`


Not: headers (plural)

Good eye! Thanks, fixed now 🙌

longquanzheng · 2024-09-27T20:13:03Z

integ/workflow/headers/routers.go

+func (h *handler) ApiV1WorkflowStateStart(c *gin.Context) {
+	headerValue := c.GetHeader(TestHeaderKey)
+	if headerValue != TestHeaderValue {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "test header not found"})


I think we need to return here after c.JSON. Otherwise the remaining code will override it.

longquanzheng · 2024-09-27T20:13:23Z

integ/workflow/headers/routers.go

+func (h *handler) ApiV1WorkflowStateDecide(c *gin.Context) {
+	headerValue := c.GetHeader(TestHeaderKey)
+	if headerValue != TestHeaderValue {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "test header not found"})


longquanzheng · 2024-09-27T20:13:39Z

integ/workflow/headers/routers.go

+	if req.GetWorkflowType() == WorkflowType {
+		h.invokeHistory[req.GetWorkflowStateId()+"_decide"]++
+		if req.GetWorkflowStateId() == State1 {
+			// go to S2


Nit: wrong comment

longquanzheng · 2024-09-27T20:14:59Z

config/config.go

@@ -68,6 +68,7 @@ type (
 		// default is http://localhost:ApiConfig.Port
 		ApiServiceAddress                  string                              `json:"serviceAddress"`
 		DumpWorkflowInternalActivityConfig *DumpWorkflowInternalActivityConfig `json:"dumpWorkflowInternalActivityConfig"`
+		DefaultHeaders                     map[string]string                   `json:"defaultHeader"`


json:"defaultHeaders"

longquanzheng · 2024-09-27T20:16:15Z

integ/workflow/headers/config.go

+const TestHeaderKey = "integration-test-header"
+const TestHeaderValue = "integration-test-value"


Nit: maybe constant.go? or just put it into routers.go

samuel27m · 2024-09-30T11:16:29Z

Thanks for the comments! Should be all good now 👍

longquanzheng · 2024-09-30T15:33:12Z

Thanks for the comments! Should be all good now 👍

Did you forget to push the changes m?

samuel27m · 2024-09-30T16:10:17Z

renaming things to be more accurate

I did push them here: af66ce5

Unless I forgot something ... 🤔

edit: i forgot to push the reset of the changes as you pointed out 😂 pushing now

samuel27m · 2024-09-30T16:11:52Z

Ok, now it should be good 😂

longquanzheng · 2024-10-01T16:59:02Z

integ/workflow/headers/constants.go

@@ -0,0 +1 @@
+package headers


Nite: we can remove it if it's empty

codecov · 2024-10-02T13:52:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.10%. Comparing base (a303990) to head (bb2ce57).
Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   72.15%   64.10%   -8.06%     
==========================================
  Files          56       57       +1     
  Lines        5043     5937     +894     
==========================================
+ Hits         3639     3806     +167     
- Misses       1138     1866     +728     
+ Partials      266      265       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

samuel27m marked this pull request as draft September 23, 2024 17:45

samuel27m requested a review from longquanzheng September 23, 2024 17:45

samuel27m commented Sep 23, 2024

View reviewed changes

samuel27m added 2 commits September 25, 2024 16:44

allow headers to be set in Interpreter Config

8a06f9c

adding integ tests

d325a39

samuel27m force-pushed the config-headers branch from c58672e to d325a39 Compare September 25, 2024 15:44

samuel27m marked this pull request as ready for review September 25, 2024 15:44

longquanzheng reviewed Sep 25, 2024

View reviewed changes

samuel27m added 2 commits September 27, 2024 15:24

Simplify workflow used for headers testing

470b97a

Merge branch 'main' into config-headers

9e723b4

longquanzheng reviewed Sep 27, 2024

View reviewed changes

DefaultHeaders in plural

a5da510

longquanzheng reviewed Sep 27, 2024

View reviewed changes

renaming things to be more accurate

af66ce5

refactoring

a26b259

longquanzheng reviewed Oct 1, 2024

View reviewed changes

longquanzheng approved these changes Oct 1, 2024

View reviewed changes

samuel27m changed the title ~~allow headers to be set in Interpreter Config~~ IWF-70: allow headers to be set in Interpreter Config Oct 2, 2024

removing empty file

bb2ce57

samuel27m merged commit 5b4cefb into main Oct 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IWF-70: allow headers to be set in Interpreter Config #424

IWF-70: allow headers to be set in Interpreter Config #424

samuel27m commented Sep 23, 2024 •

edited

Loading

samuel27m Sep 23, 2024

longquanzheng Sep 23, 2024

samuel27m Sep 24, 2024

longquanzheng Sep 24, 2024

samuel27m Sep 25, 2024 •

edited

Loading

longquanzheng Sep 25, 2024

samuel27m commented Sep 23, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

longquanzheng Sep 25, 2024

samuel27m commented Sep 27, 2024

longquanzheng Sep 27, 2024

samuel27m Sep 27, 2024

longquanzheng Sep 27, 2024

longquanzheng Sep 27, 2024

longquanzheng Sep 27, 2024

longquanzheng Sep 27, 2024

longquanzheng Sep 27, 2024

samuel27m commented Sep 30, 2024

longquanzheng commented Sep 30, 2024

samuel27m commented Sep 30, 2024 •

edited

Loading

samuel27m commented Sep 30, 2024

longquanzheng Oct 1, 2024

codecov bot commented Oct 2, 2024

		const TestHeaderKey = "integration-test-header"
		const TestHeaderValue = "integration-test-value"

IWF-70: allow headers to be set in Interpreter Config #424

IWF-70: allow headers to be set in Interpreter Config #424

Conversation

samuel27m commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel27m Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel27m commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel27m commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel27m commented Sep 30, 2024

longquanzheng commented Sep 30, 2024

samuel27m commented Sep 30, 2024 • edited Loading

samuel27m commented Sep 30, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2024

Codecov Report

samuel27m commented Sep 23, 2024 •

edited

Loading

samuel27m Sep 25, 2024 •

edited

Loading

samuel27m commented Sep 30, 2024 •

edited

Loading