Logs Tail: Give the user better feedback when --from flag errors #201
Conversation
This fixes two things: - When an unknown status code is returned we previously return from the function without and exit. This would just leave us in and endless loop doing nothing for the user. - When a user specifies a --from time that is too far in the past or future, we should let them know that so it doesn't just mysteriously give them a 404 warning and exit. Previously it would respond like the following and just hang: ``` WARNING: non-200 resp 404 ``` With this change it looks errors and exits: ``` ERROR: specified from time 1622362928 not found, either too far in the past or future ```
Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>
| // If the response was a 404, the from time was | ||
| // not valid, give them an error stating this and exit. | ||
| if resp.StatusCode == http.StatusNotFound && | ||
| c.cfg.from != 0 { |
There was a problem hiding this comment.
Sorry if I'm missing context, how does c.cfg.from != 0 mean the from date is invalid?
I fear this would catch and mislabel any 404 error?
There was a problem hiding this comment.
@jaredmorrow are you able to provide any extra context? I understand the point @ejthurgo is making but I was under the impression this logic was based on a specific behaviour inherent to your API (e.g. in the sense that the API would only return a 404 when an incorrect flag value was provided).
If that's not the case and the API is expected to return a 404 in other scenarios, then yes this could end up be an accidental match where the user provides a valid flag value but something else in their request has caused the API to return a 404.
There was a problem hiding this comment.
The combination of the 404 & the from field being set is the key here. If from is set and it is out of range, the 404 returned would be due to the range being too far out from Now. There are currently no other 404s sent by the server, but just in case I added the from != 0 to be specific about the case.
There was a problem hiding this comment.
Surprising the server doesn't give any specific detail on why it failed? Relying on the fact it currently only sends a 404 for this circumstance seems awfully risky.
But thanks for the context.
This fixes two things:
the function without and exit. This would just leave us in and endless
loop doing nothing for the user.
future, we should let them know that so it doesn't just mysteriously
give them a 404 warning and exit.
Previously it would respond like the following and just hang:
With this change it looks errors and exits: