Split parser binary out from runner #183
Conversation
| // NOTE: This code could benefit from some concurrency, but I'm opting not to prematurely optimize. | ||
| var out []*task.ParsePortfolioResponseItem | ||
| for _, p := range paths { | ||
| lineCount, err := countCSVLines(p) |
There was a problem hiding this comment.
Let's remove this - in the discussion with RMI on the doc I sent over last week we agreed that any metrics displayed to the user should come from them.
There was a problem hiding this comment.
Excellent news for you (which you've likely seen at this point): The updated parser code does surface this info, and this is indeed removed there.
| return nil | ||
| } | ||
|
|
||
| // TODO(grady): Move this line counting into the image to prevent having our code do any read of the actual underlying data. |
There was a problem hiding this comment.
Great minds - yeah lets' just remove this.
There was a problem hiding this comment.
See above
(also, not great minds, just one great mind. this is literally your comment, just moved)
| case ".ttf": | ||
| return pacta.FileType_TTF | ||
| default: | ||
| return pacta.FileType_UNKNOWN |
There was a problem hiding this comment.
Can we add a log here for missing cases?
There was a problem hiding this comment.
Indeed we do! The method is kept simple for separation of concern purposes, but we do indeed log an error if the extension is UNKNOWN
async/req.go
Outdated
| return &task, nil | ||
| } | ||
|
|
||
| func CreateReportReq() (*task.CreateReportRequest, error) { |
There was a problem hiding this comment.
nit: Consider renaming these from CreateX to something more like GetXFromEnvVars - it's just odd to see a Create method without any caller-discernable input.
| @@ -0,0 +1,71 @@ | |||
| // Package azcreds provides helpers for getting environment-appropriate credentials. | |||
There was a problem hiding this comment.
This file is super clear and really well documented. Well done!
bcspragu
force-pushed
the
brandon/parser-binary
branch
from
55b73bf to
3e2a22b
Compare
bcspragu
force-pushed
the
brandon/pacta-parse
branch
from
8a6d15e to
402b0b9
Compare
Technically, we could have used a single `runner` binary with two different base images, but that's liable to get confusing quickly, so this PR creates a new `parser` binary that is mostly just split out from the `runner`. It also refactors a few pieces of code we've been copying around: - `azcreds` - Logic for how to authenticate with Azure based on where we're running - `async` - All the business logic for running async tasks, shared between `runner` + `parser`
Since that's what we use locally.
bcspragu
force-pushed
the
brandon/parser-binary
branch
from
3e2a22b to
f47f8a6
Compare
Technically, we could have used a single
runnerbinary with two different base images, but that's liable to get confusing quickly, so this PR creates a newparserbinary that is mostly just split out from therunner.It also refactors a few pieces of code we've been copying around:
azcreds- Logic for how to authenticate with Azure based on where we're runningasync- All the business logic for running async tasks, shared betweenrunner+parser