-
Notifications
You must be signed in to change notification settings - Fork 55
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
IWF-70: allow headers to be set in Interpreter Config #424
Changes from 4 commits
8a06f9c
d325a39
470b97a
9e723b4
a5da510
af66ce5
a26b259
bb2ce57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
package integ | ||
|
||
import ( | ||
"context" | ||
"github.com/indeedeng/iwf/integ/workflow/headers" | ||
"github.com/indeedeng/iwf/service/common/ptr" | ||
"strconv" | ||
"testing" | ||
"time" | ||
|
||
"github.com/indeedeng/iwf/gen/iwfidl" | ||
"github.com/indeedeng/iwf/service" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestHeadersWorkflowTemporal(t *testing.T) { | ||
if !*temporalIntegTest { | ||
t.Skip() | ||
} | ||
|
||
for i := 0; i < *repeatIntegTest; i++ { | ||
doTestWorkflowWithHeaders(t, service.BackendTypeTemporal, nil) | ||
smallWaitForFastTest() | ||
|
||
doTestWorkflowWithHeaders(t, service.BackendTypeTemporal, minimumContinueAsNewConfig(true)) | ||
smallWaitForFastTest() | ||
|
||
doTestWorkflowWithHeaders(t, service.BackendTypeTemporal, &iwfidl.WorkflowConfig{ | ||
DisableSystemSearchAttribute: iwfidl.PtrBool(true), | ||
}) | ||
smallWaitForFastTest() | ||
} | ||
} | ||
|
||
func TestHeadersWorkflowCadence(t *testing.T) { | ||
if !*cadenceIntegTest { | ||
t.Skip() | ||
} | ||
for i := 0; i < *repeatIntegTest; i++ { | ||
doTestWorkflowWithHeaders(t, service.BackendTypeCadence, nil) | ||
smallWaitForFastTest() | ||
doTestWorkflowWithHeaders(t, service.BackendTypeCadence, minimumContinueAsNewConfig(false)) | ||
smallWaitForFastTest() | ||
} | ||
} | ||
|
||
func doTestWorkflowWithHeaders(t *testing.T, backendType service.BackendType, config *iwfidl.WorkflowConfig) { | ||
// start test workflow server | ||
wfHandler := headers.NewHandler() | ||
closeFunc1 := startWorkflowWorker(wfHandler) | ||
defer closeFunc1() | ||
|
||
_, closeFunc2 := startIwfServiceByConfig(IwfServiceTestConfig{ | ||
BackendType: backendType, | ||
DefaultHeaders: map[string]string{ | ||
headers.TestHeaderKey: headers.TestHeaderValue, | ||
}, | ||
}) | ||
defer closeFunc2() | ||
|
||
// start a workflow | ||
apiClient := iwfidl.NewAPIClient(&iwfidl.Configuration{ | ||
Servers: []iwfidl.ServerConfiguration{ | ||
{ | ||
URL: "http://localhost:" + testIwfServerPort, | ||
}, | ||
}, | ||
}) | ||
wfId := headers.WorkflowType + strconv.Itoa(int(time.Now().UnixNano())) | ||
wfInput := &iwfidl.EncodedObject{ | ||
Encoding: iwfidl.PtrString("json"), | ||
Data: iwfidl.PtrString("test data"), | ||
} | ||
req := apiClient.DefaultApi.ApiV1WorkflowStartPost(context.Background()) | ||
startReq := iwfidl.WorkflowStartRequest{ | ||
WorkflowId: wfId, | ||
IwfWorkflowType: headers.WorkflowType, | ||
WorkflowTimeoutSeconds: 100, | ||
IwfWorkerUrl: "http://localhost:" + testWorkflowServerPort, | ||
StartStateId: ptr.Any(headers.State1), | ||
StateInput: wfInput, | ||
WorkflowStartOptions: &iwfidl.WorkflowStartOptions{ | ||
WorkflowConfigOverride: config, | ||
}, | ||
} | ||
_, httpResp, err := req.WorkflowStartRequest(startReq).Execute() | ||
panicAtHttpError(err, httpResp) | ||
|
||
req2 := apiClient.DefaultApi.ApiV1WorkflowGetWithWaitPost(context.Background()) | ||
_, httpResp2, err2 := req2.WorkflowGetRequest(iwfidl.WorkflowGetRequest{ | ||
WorkflowId: wfId, | ||
}).Execute() | ||
panicAtHttpError(err2, httpResp2) | ||
|
||
assertions := assert.New(t) | ||
|
||
history, _ := wfHandler.GetTestResult() | ||
assertions.Equalf(map[string]int64{ | ||
"S1_start": 1, | ||
"S1_decide": 1, | ||
}, history, "headers test fail, %v", history) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I'm thinking yes. Do you think that would be overkill for testing such a small feature? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it’s a good idea. I have been lazy to do the right thing and the tests have become messy here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! 🙌 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I went through it and left you some comments already. and we can go through them later |
||
} | ||
|
||
func startIwfService(backendType service.BackendType) (closeFunc func()) { | ||
|
@@ -98,7 +99,6 @@ func startIwfServiceWithClient(backendType service.BackendType) (uclient uclient | |
//var integTemporalUclientCached api.UnifiedClient | ||
|
||
func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.UnifiedClient, closeFunc func()) { | ||
failAtMemoIncompatibility := !config.DisableFailAtMemoIncompatibility | ||
if config.BackendType == service.BackendTypeTemporal { | ||
dataConverter := converter.GetDefaultDataConverter() | ||
if config.MemoEncryption { | ||
|
@@ -111,7 +111,7 @@ func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.U | |
panic(err) | ||
} | ||
uclient = temporalapi.NewTemporalClient(temporalClient, testNamespace, dataConverter, config.MemoEncryption) | ||
iwfService := api.NewService(createTestConfig(failAtMemoIncompatibility, config.OptimizedVersioning), uclient, logger) | ||
iwfService := api.NewService(createTestConfig(config), uclient, logger) | ||
iwfServer := &http.Server{ | ||
Addr: ":" + testIwfServerPort, | ||
Handler: iwfService, | ||
|
@@ -123,7 +123,7 @@ func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.U | |
}() | ||
|
||
// start iwf interpreter worker | ||
interpreter := temporal.NewInterpreterWorker(createTestConfig(failAtMemoIncompatibility, config.OptimizedVersioning), temporalClient, service.TaskQueue, config.MemoEncryption, dataConverter, uclient) | ||
interpreter := temporal.NewInterpreterWorker(createTestConfig(config), temporalClient, service.TaskQueue, config.MemoEncryption, dataConverter, uclient) | ||
interpreter.Start() | ||
return uclient, func() { | ||
iwfServer.Close() | ||
|
@@ -142,7 +142,7 @@ func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.U | |
panic(err) | ||
} | ||
uclient = cadenceapi.NewCadenceClient(iwf.DefaultCadenceDomain, cadenceClient, serviceClient, encoded.GetDefaultDataConverter(), closeFunc) | ||
iwfService := api.NewService(createTestConfig(failAtMemoIncompatibility, config.OptimizedVersioning), uclient, logger) | ||
iwfService := api.NewService(createTestConfig(config), uclient, logger) | ||
iwfServer := &http.Server{ | ||
Addr: ":" + testIwfServerPort, | ||
Handler: iwfService, | ||
|
@@ -154,7 +154,7 @@ func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.U | |
}() | ||
|
||
// start iwf interpreter worker | ||
interpreter := cadence.NewInterpreterWorker(createTestConfig(failAtMemoIncompatibility, config.OptimizedVersioning), serviceClient, iwf.DefaultCadenceDomain, service.TaskQueue, closeFunc, uclient) | ||
interpreter := cadence.NewInterpreterWorker(createTestConfig(config), serviceClient, iwf.DefaultCadenceDomain, service.TaskQueue, closeFunc, uclient) | ||
interpreter.Start() | ||
return uclient, func() { | ||
iwfServer.Close() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
package headers | ||
|
||
const TestHeaderKey = "integration-test-header" | ||
const TestHeaderValue = "integration-test-value" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: maybe constant.go? or just put it into |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
package headers | ||
|
||
import ( | ||
"github.com/gin-gonic/gin" | ||
"github.com/indeedeng/iwf/gen/iwfidl" | ||
"github.com/indeedeng/iwf/service" | ||
"log" | ||
"net/http" | ||
) | ||
|
||
const ( | ||
WorkflowType = "headers" | ||
State1 = "S1" | ||
) | ||
|
||
type handler struct { | ||
invokeHistory map[string]int64 | ||
} | ||
|
||
func NewHandler() *handler { | ||
return &handler{ | ||
invokeHistory: make(map[string]int64), | ||
} | ||
} | ||
|
||
// ApiV1WorkflowStartPost - for a workflow | ||
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"}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to return here after |
||
} | ||
|
||
var req iwfidl.WorkflowStateStartRequest | ||
if err := c.ShouldBindJSON(&req); err != nil { | ||
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
return | ||
} | ||
log.Println("received state start request, ", req) | ||
|
||
if req.GetWorkflowType() == WorkflowType { | ||
// basic workflow go straight to decide methods without any commands | ||
if req.GetWorkflowStateId() == State1 { | ||
h.invokeHistory[req.GetWorkflowStateId()+"_start"]++ | ||
c.JSON(http.StatusOK, iwfidl.WorkflowStateStartResponse{ | ||
CommandRequest: &iwfidl.CommandRequest{ | ||
DeciderTriggerType: iwfidl.ALL_COMMAND_COMPLETED.Ptr(), | ||
}, | ||
}) | ||
return | ||
} | ||
} | ||
|
||
c.JSON(http.StatusBadRequest, struct{}{}) | ||
} | ||
|
||
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"}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
} | ||
|
||
var req iwfidl.WorkflowStateDecideRequest | ||
if err := c.ShouldBindJSON(&req); err != nil { | ||
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
return | ||
} | ||
log.Println("received state decide request, ", req) | ||
|
||
if req.GetWorkflowType() == WorkflowType { | ||
h.invokeHistory[req.GetWorkflowStateId()+"_decide"]++ | ||
if req.GetWorkflowStateId() == State1 { | ||
// go to S2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: wrong comment |
||
c.JSON(http.StatusOK, iwfidl.WorkflowStateDecideResponse{ | ||
StateDecision: &iwfidl.StateDecision{ | ||
NextStates: []iwfidl.StateMovement{ | ||
{ | ||
StateId: service.GracefulCompletingWorkflowStateId, | ||
StateInput: req.StateInput, | ||
}, | ||
}, | ||
}, | ||
}) | ||
return | ||
} | ||
} | ||
|
||
c.JSON(http.StatusBadRequest, struct{}{}) | ||
} | ||
|
||
func (h *handler) GetTestResult() (map[string]int64, map[string]interface{}) { | ||
return h.invokeHistory, nil | ||
} |
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.
Not: headers (plural)
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.
Good eye! Thanks, fixed now 🙌