- Notifications
You must be signed in to change notification settings - Fork1k
feat(coderd): add experimental tasks logs endpoint#19958
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
472270a toff2b3a2Compare9e17127 tob7830b0Compare562d075 todaec105CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just a small nit on thehttperror.WriteResponseError
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Danielle Maywood <danielle@themaywoods.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, some comments below but I wouldn't consider them blocking in an experimental endpoint.
| body,_:=io.ReadAll(io.LimitReader(resp.Body,128)) | ||
| returnhttperror.NewResponseError(http.StatusBadGateway, codersdk.Response{ | ||
| Message:"Task app rejected the request.", | ||
| Detail:fmt.Sprintf("Upstream status: %d; Body: %s",resp.StatusCode,body), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 this should give a good indication to the user
| case"user": | ||
| typ=codersdk.TaskLogTypeInput | ||
| case"agent": | ||
| typ=codersdk.TaskLogTypeOutput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason to have these be different? Could we instead usecodersdk.TaskLogTypeUser /codersdk.TaskLogTypeAgent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm somewhat indifferent and open to changing this, this is just the last option we discussed/settled on with@DanielleMaywood (correct me if I misremember). It seemed like a good idea to try to tie the concept ofsend input into the logs (input/output) to make the origin clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's go with this for now. We can adjust if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah that was pretty much the reason, I'm also indifferent on what we pick here as well.
91730fe toe2db31aCompare0bac5a4 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Both
followandtailhave been omitted for now.Fixescoder/internal#901