Added callback context in action execution result #5

pagrawal768 · 2024-03-11T07:18:15Z

No description provided.

metehanozturk · 2024-03-11T09:43:16Z

runbook/executor.go

 	}

-	return nil
+	waitGroup.Done()


We should not need a wait group here since running the script is a synchronous operation

metehanozturk · 2024-03-11T09:58:27Z

runbook/callback_context_reader_windows.go

+	"net"
+)
+
+var CallbackContextReaderFunc = CallbackContextReaderForWindows


you can define a CallbackContextHandler interface and implement it for linux and windows. then you can initialize the correct struct once in the runbook executor

metehanozturk · 2024-03-11T09:59:03Z

runbook/executor.go

+	var waitGroup sync.WaitGroup
+
+	var pipePath string
+	if runtime.GOOS == "windows" {


this logic should be in CallbackContextHandler

metehanozturk · 2024-03-11T10:02:18Z

runbook/executor.go

@@ -45,7 +65,7 @@ func Execute(executablePath string, args, environmentVars []string, stdout, stde
 		cmd = exec.Command(executablePath, args...)
 	}

-	cmd.Env = append(os.Environ(), environmentVars...)
+	cmd.Env = append(append(os.Environ(), fmt.Sprintf("JEC_CALLBACK_PIPE_PATH=%s", pipePath)), environmentVars...)


won't JEC_CALLBACK_PIPE_PATH be overriden by each worker concurrently? we have multiple workers running at the same time. better to add it into args

os.Environ() returns a copy of environment variables, so it will not be overwritten. Though I will add it in args

metehanozturk · 2024-03-11T10:09:19Z

runbook/executor.go

+	pipe := CreatePipeFunc(pipePath)
+
+	waitGroup.Add(1)
+	go CallbackContextReaderFunc(callbackContextBuffer, pipe)


not sure if we need a separate go routine here. is it a significant performance improver?

Actually its not for performance improvement. when we open the named pipe for reading (or for windows when we call listener.Accept()) the process will stop there until any other process opens the other end of pipe for writing the data.
So with this go routine we will parallelly start waiting for pipe write and the main routine will continue to execute the script.
Before the script execution started we have already created the pipe and waiting for data to be written. Now inside script execution we can open the pipe and write data to it.

If the script has written some data then the go routine will terminate after reading it. But if the script has not opened the pipe and finished execution then we can call waitGroup.Done() to stop that go rountine.

do we need to listen to the pipe in parallel? do we have an option like open file -> execute -> read all -> close conn?

metehanozturk · 2024-03-11T10:21:41Z

queue/message_test.go

@@ -63,8 +63,8 @@ var mockActionLoggers = map[string]io.Writer{
 	"/path/to/stderr": mockStderr,
 }

-func mockExecute(executablePath string, args, environmentVars []string, stdout, stderr io.Writer) error {
-	return nil
+func mockExecute(messageId string, executablePath string, args, environmentVars []string, stdout, stderr io.Writer) (string, error) {


messageId is not a meaningful naming in the execution context. it might be executionId instead.

metehanozturk · 2024-03-11T10:23:07Z

go.mod

 	github.com/pkg/errors v0.8.1
 	github.com/prometheus/client_golang v1.1.0
 	github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4 // indirect
 	github.com/prometheus/procfs v0.0.4 // indirect
-	github.com/sirupsen/logrus v1.4.2
+	github.com/sirupsen/logrus v1.2.0


is it downgraded?

seems like it got downgraded by winio package. upgrading it back to 1.4.2

metehanozturk · 2024-03-11T10:34:02Z

queue/message.go

@@ -102,18 +104,18 @@ func (mh *messageHandler) Handle(message sqs.Message) (*runbook.ActionResultPayl
 	return result, nil
 }

-func (mh *messageHandler) execute(mappedAction *conf.MappedAction, messageBody string) (string, error) {
+func (mh *messageHandler) execute(mappedAction *conf.MappedAction, message sqs.Message) (string, string, error) {


you can use a pointer for sqs message not to copy the value. also it is an already private method

metehanozturk · 2024-03-12T07:56:10Z

go.mod

 	github.com/google/uuid v1.1.1
 	github.com/kardianos/service v1.0.0
-	github.com/konsorten/go-windows-terminal-sequences v1.0.2 // indirect


why are the indirect ones removed? also, I guess go-windows-terminal-sequences is downgraded

these were unused packages which got removed after running go mod tidy

metehanozturk · 2024-03-12T07:58:05Z

runbook/callback_context_reader.go

@@ -0,0 +1,7 @@
+package runbook
+
+type CallbackContextReader interface {


It is not just "reader". maybe "handler" or other generic wording could be good

metehanozturk · 2024-03-12T07:59:20Z

runbook/callback_context_reader_unix.go

+	data, err := ioutil.ReadAll(file)
+	_ = copy(callbackContextReader.callbackContextBuffer, data)
+	if err != nil {
+		fmt.Println("Error reading from the named pipe:", err)


you can use logrus instead of fmt

metehanozturk · 2024-03-12T08:09:06Z

runbook/executor.go

+
+	var waitGroup sync.WaitGroup
+
+	waitGroup.Add(1)


no one is waiting for this waitGroup. what is the purpose?

metehanozturk · 2024-04-02T17:04:35Z

runbook/executor.go

 	}

-	return nil
+	callbackContext := bytes.NewBuffer(bytes.Trim(callbackContextHandler.callbackContextBuffer, "\x00")).String()
+	logrus.Debug("Callback context: " + callbackContext)


should be removed

metehanozturk · 2024-04-05T10:29:17Z

vendor/modules.txt

@@ -111,7 +113,7 @@ github.com/jmespath/go-jmespath
 github.com/kardianos/service
 # github.com/kevinburke/ssh_config v0.0.0-20190725054713-01f96b0aa0cd
 github.com/kevinburke/ssh_config
-# github.com/konsorten/go-windows-terminal-sequences v1.0.2


why is the version downgraded?

pagrawal768 requested a review from metehanozturk March 11, 2024 07:18

pagrawal768 force-pushed the BIF-1000 branch 4 times, most recently from 702ee06 to 89029d6 Compare March 11, 2024 08:53

metehanozturk reviewed Mar 11, 2024

View reviewed changes

pagrawal768 force-pushed the BIF-1000 branch 3 times, most recently from 69aab9d to 50599a0 Compare March 12, 2024 06:11

metehanozturk reviewed Mar 12, 2024

View reviewed changes

pagrawal768 force-pushed the BIF-1000 branch 5 times, most recently from 4cb9c4c to f02510c Compare March 12, 2024 16:46

pagrawal768 added 2 commits March 18, 2024 23:46

Added callback context in action execution result

a25767b

Adding vendor package updates

a242bee

pagrawal768 force-pushed the BIF-1000 branch from f02510c to a242bee Compare March 18, 2024 18:16

metehanozturk requested changes Apr 5, 2024

View reviewed changes

metehanozturk approved these changes May 7, 2024

View reviewed changes

ertugrulsmz approved these changes May 7, 2024

View reviewed changes

metehanozturk merged commit 66b5f85 into master May 7, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added callback context in action execution result #5

Added callback context in action execution result #5

pagrawal768 commented Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 11, 2024

pagrawal768 Mar 11, 2024

metehanozturk Mar 11, 2024

pagrawal768 Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 11, 2024

pagrawal768 Mar 11, 2024

metehanozturk Mar 11, 2024

metehanozturk Mar 12, 2024

pagrawal768 Mar 12, 2024

metehanozturk Mar 12, 2024

metehanozturk Mar 12, 2024

metehanozturk Mar 12, 2024

metehanozturk Apr 2, 2024

metehanozturk Apr 5, 2024

		@@ -0,0 +1,7 @@
		package runbook

		type CallbackContextReader interface {

Added callback context in action execution result #5

Added callback context in action execution result #5

Conversation

pagrawal768 commented Mar 11, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment