Channel Refactoring #91

isatasan · 2021-08-04T18:41:46Z

No description provided.

msarvar · 2021-08-09T17:47:42Z

server/server.go

@@ -277,7 +279,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 	}
 	vcsClient := vcs.NewClientProxy(githubClient, gitlabClient, bitbucketCloudClient, bitbucketServerClient, azuredevopsClient)
 	commitStatusUpdater := &events.DefaultCommitStatusUpdater{Client: vcsClient, StatusName: userConfig.VCSStatusName}
-	terraformOutputChan := make(chan *models.TerraformOutputLine)
+	terraformOutputChan := make(chan *models.ProjectCmdOutputLine)


Suggested change

terraformOutputChan := make(chan *models.ProjectCmdOutputLine)

projectCmdOutput := make(chan *models.ProjectCmdOutputLine)

server/server.go

msarvar · 2021-08-09T17:54:46Z

server/events/terraform/terraform_client.go

+	terraformOutputChan     chan<- *models.ProjectCmdOutputLine
+	ProjectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler


Suggested change

terraformOutputChan chan<- *models.ProjectCmdOutputLine

ProjectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler

projectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler

replace every struct that uses terraformOutputChan with ProjectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler

msarvar · 2021-08-09T17:55:00Z

server/events/terraform/terraform_client.go

@@ -105,7 +107,7 @@ func NewClientWithDefaultVersion(
 	tfDownloader Downloader,
 	usePluginCache bool,
 	fetchAsync bool,
-	terraformOutputChan chan<- *models.TerraformOutputLine,
+	terraformOutputChan chan<- *models.ProjectCmdOutputLine,


update this to projectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler

Suggested change

terraformOutputChan chan<- *models.ProjectCmdOutputLine,

projectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler,

msarvar · 2021-08-09T17:55:34Z

server/events/terraform/terraform_client.go

@@ -173,6 +175,7 @@ func NewClientWithDefaultVersion(
 		versions:                versions,
 		usePluginCache:          usePluginCache,
 		terraformOutputChan:     terraformOutputChan,
+		ProjectCmdOutputHandler: handlers.DefaultProjectCommandOutputHandler{},


Suggested change

ProjectCmdOutputHandler: handlers.DefaultProjectCommandOutputHandler{},

ProjectCmdOutputHandler: projectCmdOutputHandler,

msarvar · 2021-08-09T17:57:14Z

So all structs that have TerraformOutputChan attribute should be updated to use handlers.ProjectCommandOutputHandler instead. You should remove TerraformOutputChan and use ProjectCommandOutputHandler instead.

msarvar · 2021-08-09T19:03:29Z

server/server.go

@@ -508,7 +511,9 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		WorkingDir:          workingDir,
 		Webhooks:            webhooksManager,
 		WorkingDirLocker:    workingDirLocker,
-		TerraformOutputChan: terraformOutputChan,
+		ProjectCmdOutputHandler: handlers.DefaultProjectCommandOutputHandler{


why are you defining a new handlers.DefaultProjectCommandOutputHandler? You already created it pass in projectCmdOutputHandler

msarvar · 2021-08-09T19:04:33Z

server/server.go

@@ -302,7 +307,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		userConfig.TFDownloadURL,
 		&terraform.DefaultDownloader{},
 		true,
-		terraformOutputChan)
+		*projectCmdOutputHandler)


Suggested change

*projectCmdOutputHandler)

projectCmdOutputHandler)

msarvar · 2021-08-09T19:29:50Z

server/controllers/logstreaming_controller.go

-	chanLock         sync.RWMutex
-	WebsocketHandler WebsocketHandler
+	WebsocketHandler            handlers.WebsocketHandler
+	ProjectCommandOutputHandler handlers.DefaultProjectCommandOutputHandler


Suggested change

ProjectCommandOutputHandler handlers.DefaultProjectCommandOutputHandler

ProjectCommandOutputHandler handlers.ProjectCommandOutputHandler

msarvar · 2021-08-09T19:32:00Z

server/controllers/events/events_controller_e2e_test.go

@@ -666,7 +667,10 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 	e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient}
 	e2eGithubGetter := mocks.NewMockGithubPullGetter()
 	e2eGitlabGetter := mocks.NewMockGitlabMergeRequestGetter()
-	tempchan := make(chan *models.TerraformOutputLine)
+	tempchan := make(chan *models.ProjectCmdOutputLine)
+	projectCmdOutputHandler := handlers.DefaultProjectCommandOutputHandler{


Suggested change

projectCmdOutputHandler := handlers.DefaultProjectCommandOutputHandler{

projectCmdOutputHandler := &handlers.DefaultProjectCommandOutputHandler{

msarvar · 2021-08-09T22:07:51Z

server/controllers/events/events_controller_e2e_test.go

@@ -666,7 +667,10 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 	e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient}
 	e2eGithubGetter := mocks.NewMockGithubPullGetter()
 	e2eGitlabGetter := mocks.NewMockGitlabMergeRequestGetter()
-	tempchan := make(chan *models.TerraformOutputLine)
+	tempchan := make(chan *models.ProjectCmdOutputLine)
+	projectCmdOutputHandler := &handlers.DefaultProjectCommandOutputHandler{


For tests you should generate mocks and use them instead of directly creating handlers.DefaultProjectCommandOutputHandler object.
Same way we are mocking WebsocketHandler

server/handlers/project_command_output_handler.go

msarvar · 2021-08-09T22:17:20Z

server/handlers/project_command_output_handler.go

+		err := callback(msg)
+		if err != nil {
+			p.logger.Err(err.Error())
+		}
+		return nil


Suggested change

err := callback(msg)

if err != nil {

p.logger.Err(err.Error())

}

return nil

if err := callback(msg); err != nil {

return err

}

msarvar · 2021-08-09T22:19:14Z

server/handlers/project_command_output_handler.go

+func NewProjectCommandOutputHandler() ProjectCommandOutputHandler {
+	return &DefaultProjectCommandOutputHandler{
+		ProjectCmdOutput: make(chan *models.ProjectCmdOutputLine),
+	}
+}


Suggested change

func NewProjectCommandOutputHandler() ProjectCommandOutputHandler {

return &DefaultProjectCommandOutputHandler{

ProjectCmdOutput: make(chan *models.ProjectCmdOutputLine),

}

}

func NewProjectCommandOutputHandler(projectCmdOutput chan *models.ProjectCmdOutputLine, logger logging.SimpleLogging) ProjectCommandOutputHandler {

return &DefaultProjectCommandOutputHandler{

ProjectCmdOutput: projectCmdOutput,

logger: logger,

controllerBuffer: map[string]map[chan string]bool{},

}

}

Use this function in the server.go

msarvar · 2021-08-10T21:17:22Z

server/controllers/events/events_controller_e2e_test.go

@@ -666,7 +667,8 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 	e2eStatusUpdater := &events.DefaultCommitStatusUpdater{Client: e2eVCSClient}
 	e2eGithubGetter := mocks.NewMockGithubPullGetter()
 	e2eGitlabGetter := mocks.NewMockGitlabMergeRequestGetter()
-	tempchan := make(chan *models.TerraformOutputLine)
+	tempchan := make(chan *models.ProjectCmdOutputLine)


You can remove this

msarvar · 2021-08-10T21:19:36Z

server/controllers/logstreaming_controller_test.go

+		projectOutputHandler.Send(ctx, "Test Terraform Output")
+		projectOutputHandler.Handle()


I don't think you need these two lines.

msarvar · 2021-08-10T21:20:07Z

server/events/project_command_runner.go

@@ -119,11 +121,12 @@ type DefaultProjectCommandRunner struct {
 	VersionStepRunner          StepRunner
 	RunStepRunner              CustomStepRunner
 	EnvStepRunner              EnvStepRunner
+	PullApprovedChecker        runtime.PullApprovedChecker


Suggested change

PullApprovedChecker runtime.PullApprovedChecker

msarvar · 2021-08-10T21:20:18Z

server/events/project_command_runner_test.go

+		ApplyStepRunner:         mockApply,
+		RunStepRunner:           mockRun,
+		EnvStepRunner:           &realEnv,
+		PullApprovedChecker:     nil,


Suggested change

PullApprovedChecker: nil,

msarvar · 2021-08-10T21:21:57Z

server/server.go

@@ -508,10 +511,11 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		EnvStepRunner: &runtime.EnvStepRunner{
 			RunStepRunner: runStepRunner,
 		},
+		PullApprovedChecker:        vcsClient,


Suggested change

PullApprovedChecker: vcsClient,

msarvar · 2021-08-10T21:22:07Z

server/controllers/events/events_controller_e2e_test.go

-		WorkingDir:       workingDir,
-		Webhooks:         &mockWebhookSender{},
-		WorkingDirLocker: locker,
+		PullApprovedChecker:     e2eVCSClient,


Suggested change

PullApprovedChecker: e2eVCSClient,

msarvar · 2021-08-10T21:33:22Z

server/handlers/project_command_output_handler.go

+func (p *DefaultProjectCommandOutputHandler) writeLogLine(pull string, line string) {
+	p.controllerBufferLock.Lock()
+	if p.projectOutputBuffers == nil {
+		p.projectOutputBuffers = map[string][]string{}


you can move this into NewProjectCommandOutputHandler

msarvar · 2021-08-10T21:34:45Z

server/handlers/project_command_output_handler.go

+//Remove channel, so client no longer receives Terraform output
+func (p *DefaultProjectCommandOutputHandler) removeChan(pull string, ch chan string) {
+	p.controllerBufferLock.Lock()
+	delete(p.controllerBuffers[pull], ch)


We should also close the channel here.

Suggested change

delete(p.controllerBuffers[pull], ch)

delete(p.controllerBuffers[pull], ch)

close(ch)

msarvar · 2021-08-10T21:35:09Z

server/handlers/project_command_output_handler.go

+	if p.controllerBuffers == nil {
+		p.controllerBuffers = map[string]map[chan string]bool{}
+	}


Suggested change

if p.controllerBuffers == nil {

p.controllerBuffers = map[string]map[chan string]bool{}

}

msarvar · 2021-08-10T21:47:12Z

server/handlers/project_command_output_handler.go

+	// this logBuffers
+	projectOutputBuffers map[string][]string
+	// this is wsChans
+	controllerBuffers map[string]map[chan string]bool


Let's rename this into receiverBuffers

Suggested change

controllerBuffers map[string]map[chan string]bool

receiverBuffers map[string]map[chan string]bool

msarvar · 2021-08-10T22:40:09Z

server/handlers/project_command_output_handler.go

+	return &DefaultProjectCommandOutputHandler{
+		ProjectCmdOutput:  projectCmdOutput,
+		logger:            logger,
+		controllerBuffers: map[string]map[chan string]bool{},


Suggested change

controllerBuffers: map[string]map[chan string]bool{},

controllerBuffers: map[string]map[chan string]bool{},

projectOutputBuffers: map[string][]string{},

msarvar · 2021-08-10T22:59:09Z

server/handlers/project_command_output_handler.go

+	if p.projectOutputBuffers == nil {
+		p.projectOutputBuffers = map[string][]string{}
+	}


Suggested change

if p.projectOutputBuffers == nil {

p.projectOutputBuffers = map[string][]string{}

}

msarvar · 2021-08-12T22:27:16Z

server/handlers/project_command_output_handler.go

-		ProjectInfo: ctx.PullInfo(),
-		Line:        msg,
+		ProjectInfo:     ctx.PullInfo(),
+		ClearBuffBefore: true,


You don't want to set this to true everytime you send a message. You want to clear the buffer before starting terraform process on a project. You can create a Clear(ctx models.ProjectCommandContext, msg string) function that will set the flag to true

msarvar · 2021-08-16T19:25:53Z

server/handlers/project_command_output_handler_test.go

+		go func() {
+			projectOutputHandler.Receive(ctx.PullInfo(), func(msg string) error {
+				expectMsg = msg
+				wg.Done()


you can move this outside of receive callback and put as a last thing in the go routine. You are sending 3 messages to the queue. So it is calling wg.Done() 3 times. But wait group is only expecting 1 call.

server/handlers/project_command_output_handler_test.go

msarvar · 2021-08-16T21:50:07Z

server/handlers/project_command_output_handler_test.go

+		go func() {
+			projectOutputHandler.Send(ctx, "Test Terraform Output")
+		}()
+
+		go func() {
+			projectOutputHandler.Clear(ctx, "")
+		}()
+
+		go func() {
+			projectOutputHandler.Send(ctx, "Test Terraform Output")
+		}()
+
+		expectMsg := ""
+		wg.Add(1)
+		go func() {
+			projectOutputHandler.Receive(ctx.PullInfo(), func(msg string) error {
+				expectMsg = msg
+				return nil
+			})
+			wg.Done()
+		}()
+		wg.Wait()


This should fix the test

Suggested change

go func() {

projectOutputHandler.Send(ctx, "Test Terraform Output")

}()

go func() {

projectOutputHandler.Clear(ctx, "")

}()

go func() {

projectOutputHandler.Send(ctx, "Test Terraform Output")

}()

expectMsg := ""

wg.Add(1)

go func() {

projectOutputHandler.Receive(ctx.PullInfo(), func(msg string) error {

expectMsg = msg

return nil

})

wg.Done()

}()

wg.Wait()

wg.Add(1)

go func() {

projectOutputHandler.Send(ctx, "Test Terraform Output")

}()

wg.Add(1)

go func() {

projectOutputHandler.Clear(ctx, "")

}()

wg.Add(1)

go func() {

projectOutputHandler.Send(ctx, "Test Terraform Output")

}()

expectMsg := ""

go func() {

projectOutputHandler.Receive(ctx.PullInfo(), func(msg string) error {

expectMsg = msg

wg.Done()

return nil

})

}()

wg.Wait()

* Began channel refactoring * Fixing refactoring logic * Replacing terraformOutputChan * Making suggested changes * Changing attribute declarations * Fixed channel logic * Formatting * Changing tests to utilize mocks * Adding go files * Adding suggested changes * Suggested changes before merging * Reformatting * Deleting unnecesary lines * Buffer cleanup logic * Buffer cleanup & Testing * Trying to make test pass * Suggested changes * Fixing test * Fixing failing test * Error checking * Comment improvements * Adding buffer clearing for apply workflow * Removing clearing for apply workflow

isatasan added 2 commits August 4, 2021 12:13

Began channel refactoring

f7a728b

Fixing refactoring logic

47b0016

msarvar reviewed Aug 9, 2021

View reviewed changes

server/server.go Show resolved Hide resolved

msarvar reviewed Aug 9, 2021

View reviewed changes

Replacing terraformOutputChan

0851e81

msarvar reviewed Aug 9, 2021

View reviewed changes

Making suggested changes

afb6439

msarvar reviewed Aug 9, 2021

View reviewed changes

Changing attribute declarations

3726505

msarvar reviewed Aug 9, 2021

View reviewed changes

server/handlers/project_command_output_handler.go Show resolved Hide resolved

msarvar reviewed Aug 9, 2021

View reviewed changes

isatasan added 5 commits August 9, 2021 18:43

Fixed channel logic

6a7e458

Formatting

90bc651

Changing tests to utilize mocks

7547634

Adding go files

b2370b1

Fixing merge conflicts

64d4974

msarvar reviewed Aug 10, 2021

View reviewed changes

Adding suggested changes

618f5c3

msarvar reviewed Aug 10, 2021

View reviewed changes

isatasan added 2 commits August 10, 2021 18:44

Suggested changes before merging

93ab842

Reformatting

a4c0e67

msarvar reviewed Aug 10, 2021

View reviewed changes

isatasan added 2 commits August 10, 2021 19:02

Deleting unnecesary lines

8f1504c

Buffer cleanup logic

d98dee1

msarvar reviewed Aug 12, 2021

View reviewed changes

Buffer cleanup & Testing

fa1f727

msarvar reviewed Aug 16, 2021

View reviewed changes

Trying to make test pass

6b47666

msarvar reviewed Aug 16, 2021

View reviewed changes

server/handlers/project_command_output_handler_test.go Outdated Show resolved Hide resolved

Suggested changes

35d19d0

msarvar reviewed Aug 16, 2021

View reviewed changes

isatasan added 6 commits August 16, 2021 18:02

Fixing test

5fbe4f6

Fixing failing test

1380ad3

Error checking

8e5e05d

Comment improvements

828e3b0

Adding buffer clearing for apply workflow

4d442a9

Removing clearing for apply workflow

804c6ba

msarvar approved these changes Aug 17, 2021

View reviewed changes

msarvar merged commit a44c966 into release-v0.17.1-lyft.1 Aug 17, 2021

msarvar deleted the Refactoring branch August 17, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Channel Refactoring #91

Channel Refactoring #91

isatasan commented Aug 4, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar commented Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021

msarvar Aug 9, 2021 •

edited

Loading

msarvar Aug 9, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 10, 2021

msarvar Aug 12, 2021

msarvar Aug 16, 2021

msarvar Aug 16, 2021

	terraformOutputChan := make(chan *models.ProjectCmdOutputLine)
	projectCmdOutput := make(chan *models.ProjectCmdOutputLine)

		terraformOutputChan chan<- *models.ProjectCmdOutputLine
		ProjectCmdOutputHandler handlers.DefaultProjectCommandOutputHandler

	ProjectCmdOutputHandler: handlers.DefaultProjectCommandOutputHandler{},
	ProjectCmdOutputHandler: projectCmdOutputHandler,

	ProjectCommandOutputHandler handlers.DefaultProjectCommandOutputHandler
	ProjectCommandOutputHandler handlers.ProjectCommandOutputHandler

	projectCmdOutputHandler := handlers.DefaultProjectCommandOutputHandler{
	projectCmdOutputHandler := &handlers.DefaultProjectCommandOutputHandler{

		projectOutputHandler.Send(ctx, "Test Terraform Output")
		projectOutputHandler.Handle()

	delete(p.controllerBuffers[pull], ch)
	delete(p.controllerBuffers[pull], ch)
	close(ch)

	if p.controllerBuffers == nil {
	p.controllerBuffers = map[string]map[chan string]bool{}
	}

	controllerBuffers map[string]map[chan string]bool
	receiverBuffers map[string]map[chan string]bool

	controllerBuffers: map[string]map[chan string]bool{},
	controllerBuffers: map[string]map[chan string]bool{},
	projectOutputBuffers: map[string][]string{},

	if p.projectOutputBuffers == nil {
	p.projectOutputBuffers = map[string][]string{}
	}

Channel Refactoring #91

Channel Refactoring #91

Conversation

isatasan commented Aug 4, 2021

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

msarvar commented Aug 9, 2021

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

msarvar Aug 9, 2021 • 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

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

msarvar Aug 9, 2021 •

edited

Loading