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.

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

Orca 845 #89

Merged
merged 34 commits into from
Aug 2, 2021
Merged

Orca 845 #89

merged 34 commits into from
Aug 2, 2021

Conversation

msarvar
Copy link

@msarvar msarvar commented Jul 15, 2021

No description provided.

wsChans map[string]map[chan string]bool
chanLock sync.RWMutex
WebsocketHandler WebsocketHandler
WebsocketResponseWriter WebsocketResponseWriter
Copy link
Author

Choose a reason for hiding this comment

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

You don't need this.

server/server.go Outdated
@@ -616,6 +624,18 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
DB: boltdb,
DeleteLockCommand: deleteLockCommand,
}
var websocketHandler controllers.WebsocketHandler
Copy link
Author

@msarvar msarvar Jul 19, 2021

Choose a reason for hiding this comment

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

You're declaring a variable that is interface of type controllers.WebsocketHandler, but there is no assignment that implements this interface. Otherwise you're just passing nil pointers around.
You need to declare create a struct that implements controllers.WebsocketHandler interface. The struct itself is just a delegate Upgrade method to instance of websocket.Upgrader{}. Something like this:

type DefaultWebsocketHandler struct {
  handler websocket.Upgrader
}

func NewWebsocketHandler() WebsocketHandler {
  return &DefaultWebsocketHandler{
    handler: websocket.Upgrader{},
  }
}

func (wh *DefaultWebsocketHandler) Upgrade(w http.ResponseWriter, r *http.Request, responseHeader http.Header) (WebsocketResponseWriter, error) {
  return wh.handler.Upgrade(w, r, responseHeader)
}

ctx := models.ProjectCommandContext{
Log: logging.NewNoopLogger(t),
Copy link
Author

Choose a reason for hiding this comment

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

you don't need to change these

@msarvar
Copy link
Author

msarvar commented Jul 30, 2021

As we discussed we should refactor our logic around how we are sending log streaming to the controller. Right now we are passing in the TerraformOutputLine channel into each struct.

Let's create an interface called ProjectCommandOutputHandler. It will implement sending and receiving messages and wrap the channel.
It should implement 3 functions.

type ProjectCommandOutputHandler interface {
  // Send will enqueue the msg and wait for Handle() to receive the message.
  // this will be called in the RunCommandAsync where we currently sending messages to the terraform channel
  Send(ctx models.ProjectCommandContext, msg string) error

  // Receive will create a channel for projectPullInfo and run callback function argument when the new channel receives a message. This will be called from the controller. 
  // j.ProjectCommandOutputHandler.Receive(pull, func(msg string) {
  //   j.Logger.Info(msg)
  //   if err := c.WriteMessage(websocket.BinaryMessage, []byte(msg+"\r\n\t")); err != nil {
  //     j.Logger.Warn("Failed to write ws message: %s", err)
  //     return err
  //   }
  // })
  Receive(projectPullInfo string, callback func(msg string) error) error
  // Is basically the same as Listen function from logstreaming controller.
  Handle() 
}

To implement the interface let's create a struct DefaultProjectCommandOutputHandler.

type DefaultProjectCommandOutputHandler struct {
    // this is TerraforOutputChan
    projectCmdOutput chan *models.ProjectCmdOutputLine
   // this logBuffers
    projectOutputBuffers       map[string][]string
    // this is wsChans
    controllerBuffers          map[string]map[chan string]bool
    // same as chanLock
    controllerBufferLock         sync.RWMutex
}

"project": "test-project",
}
ctx := context.Background()
ctx = context.WithValue(ctx, int(0), params)
Copy link
Author

Choose a reason for hiding this comment

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

True removing line 33 and 34. I don't think we need them anymore.

Comment on lines 188 to 189
err = j.LogStreamErrorTemplate.Execute(w, err)
if err != nil {
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
err = j.LogStreamErrorTemplate.Execute(w, err)
if err != nil {
if err := j.LogStreamErrorTemplate.Execute(w, err); err != nil {

@msarvar msarvar merged commit 4779801 into release-v0.17.1-lyft.1 Aug 2, 2021
@msarvar msarvar deleted the orca-845 branch August 2, 2021 18:45
msarvar added a commit that referenced this pull request Sep 27, 2021
* Current progress

* Websocket implementation

* Fixing routes

* Adding url

* Fixed websocket connection

* Progress 7/9

* Added some channel logic

* Changes to models.go

* Working on log streaming for plan

* Beginning tests/Having errors

* Log Streaming Logic

* fixed test

* Added Testing

* Fixed websocket connection

* Async implementation and test error fixes

* Changes to variables in tests

* Debugging

* Fixed UI in webpage terminal

* Fixing Run make-test coverage

* Fixing tests

* Testing

* Deleted txt.act files

* Fixing broken tests

* Changes to terraform_client testing

* Fixed tests in terraform_client_internal_test.go

* Fixed failing test

* Deleted lines causing test to fail

* Reformating

* More Reformatting

* Run make check-lint corrections

* Added error check

* Fixing check-lint test

* Suggested changes prior to merge

Co-authored-by: Isata Sankoh <[email protected]>
Aayyush pushed a commit that referenced this pull request Dec 9, 2021
* Current progress

* Websocket implementation

* Fixing routes

* Adding url

* Fixed websocket connection

* Progress 7/9

* Added some channel logic

* Changes to models.go

* Working on log streaming for plan

* Beginning tests/Having errors

* Log Streaming Logic

* fixed test

* Added Testing

* Fixed websocket connection

* Async implementation and test error fixes

* Changes to variables in tests

* Debugging

* Fixed UI in webpage terminal

* Fixing Run make-test coverage

* Fixing tests

* Testing

* Deleted txt.act files

* Fixing broken tests

* Changes to terraform_client testing

* Fixed tests in terraform_client_internal_test.go

* Fixed failing test

* Deleted lines causing test to fail

* Reformating

* More Reformatting

* Run make check-lint corrections

* Added error check

* Fixing check-lint test

* Suggested changes prior to merge

Co-authored-by: Isata Sankoh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants