-
Notifications
You must be signed in to change notification settings - Fork 2k
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
api: add follow param to file stream endpoint #6049
Conversation
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.
Yay - first PR! Found a minor style nit-picking and test condition, but not blockers. Merge away if you feel you've addressed them.
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.
very minor nitpick, looks good to me 🎉
you can update the documentation /api/client.html#stream-file and changelog in this PR or a follow-up PR.
command/agent/fs_endpoint.go
Outdated
@@ -210,10 +213,15 @@ func (s *HTTPServer) Stream(resp http.ResponseWriter, req *http.Request) (interf | |||
return nil, fileNameNotPresentErr | |||
} | |||
|
|||
if followStr := q.Get("follow"); followStr != "" { | |||
if follow, err = strconv.ParseBool(followStr); err != nil { | |||
return nil, fmt.Errorf("Failed to parse follow field to boolean: %v", err) |
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.
we strive (but sometimes fail) to start error messages with lower-case
293d212
to
8c95660
Compare
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
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
IMPROVEMENTS: | |||
* agent: allow the job GC interval to be configured [[GH-5978](https://github.com/hashicorp/nomad/issues/5978)] | |||
* api: add follow parameter to file streaming endpoint to support older browsers [[GH-6049](https://github.com/hashicorp/nomad/pull/6049] |
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 think this is missing a closing parenthesis
these are easy to mess up by copy-pasting. we tend to use [GH-####]
and then run make changelogfmt
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.
Ah, thanks for catching that. Done.
I'll merge this once CI gives the 👍 again.
The `/v1/client/fs/stream endpoint` supports tailing a file by writing chunks out as they come in. But not all browsers support streams (ex IE11) so we need to be able to tail a file without streaming. The fs stream and logs endpoint use the same implementation for filesystem streaming under the hood, but the fs stream always passes the `follow` parameter set to true. This adds the same toggle to the fs stream endpoint that we have for logs. It defaults to true for backwards compatibility.
8c95660
to
41dc6ba
Compare
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. Merge away :).
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
The
/v1/client/fs/stream endpoint
supports tailing a file by writing chunks out as they come in. But not all browsers support streams (ex IE11) so we need to be able to tail a file without streaming.The fs stream and logs endpoint use the same implementation for filesystem streaming under the hood, but the fs stream always passes the
follow
parameter set to true. This adds the same toggle to the fs stream endpoint that we have for logs. It defaults to true for backwards compatibility.