Refactor log handler #175
Refactor log handler #175
Conversation
Aayyush
commented
Feb 3, 2022
Aayyush
commented
- Move Job url generation into it's own component to reduce the responsibility on ProjectCommandOuptutHandler
- Move log streaming source files into server/jobs and refactor out log streaming specific models into its own models package under jobs directory
| Line string | ||
| OperationComplete bool | ||
| } | ||
|
|
||
| // AsyncProjectCommandOutputHandler is a handler to transport terraform client | ||
| // outputs to the front end. | ||
| type AsyncProjectCommandOutputHandler struct { |
There was a problem hiding this comment.
why is this still in a /handlers/ package? doesn't seem like it needs to be now. I don't like the name handler in general because it's very vague as to what the respondibility actually is.
There was a problem hiding this comment.
True. I'll remove the handlers directory and move it to the jobs package. Should we rename the struct and the interface as well? From my understanding, there's two key responsibilities of the AsyncProjectCommandOutputHandler: in memory registry for logs and output logs handler. We could just name it registry but that would not account for the handling of logs. Do you have any naming suggestions?
There was a problem hiding this comment.
You can probably just leave that for now since it existed before (I also don't have great suggestions at this point)
server/jobs/job_url_setter.go
Outdated
| SetJobURLWithStatus(ctx models.ProjectCommandContext, cmdName models.CommandName, status models.CommitStatus) error | ||
| } | ||
|
|
||
| type DefaultJobURLSetter struct { |
There was a problem hiding this comment.
See comment about prepending default to interface names:
../atlantis/internal/team_practices/style_guide.html#interface-naming
There was a problem hiding this comment.
Moved the JobURLSetter interface to project_command_runner (where it's being used) and renamed DefaultJobUrlSetter to JobURLSetter.
|
|
||
| //go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_project_status_updater.go ProjectStatusUpdater | ||
|
|
||
| type ProjectStatusUpdater interface { |
There was a problem hiding this comment.
I want us to get away from this practice of exporting interfaces because it creates an unnecessary dependency between packages. Also it's more semantic in go to always return a concrete type and use an interface on the reader side.
Here's an example of this: https://stackoverflow.com/questions/51031116/export-interfaces-instead-of-structs-from-golang-packages
There was a problem hiding this comment.
Yeah I like this approach too! I've moved the JobURLSetter to project_command_runner and left the unexported interfaces ProjectJobURLGenerator and ProjectStatusUpdater as it is.
server/jobs/models/models.go
Outdated
| @@ -0,0 +1,25 @@ | |||
| package models | |||
|
|
|||
| type OutputBuffer struct { | |||
There was a problem hiding this comment.
I think i commented about this in the last PR, I don't agree with the concept of using models as a filename or a package name because it just becomes a dumping ground of things eventually. Let's have names that make sense.
In this case the objects are also arbitrarily appended with Context so maybe an overall rethink on naming here is a good idea.
There was a problem hiding this comment.
I've moved the structs to project_command_output_handler.go since that's the only place they are being used and I've renamed PullContext and JobContext to PullInfo and JobInfo respectively. Lmk if you have any other naming suggestions 😅
* Add UUID for Log Streaming Job ID (#167) * Update log handler to close buffered channels when an operation is complete (#170) * Add preliminary check before registering new receivers in the log handler (#173) * Using projectOutputBuffers to check for jobID instead of receiverBuffers (#181) * Refactor log handler (#175) * Reverting go.mod and go.sum * Fix lint errors * Fix linting
