s3stream: follow CI logs from the CLI#9274
Conversation
| ) | ||
| bytes_received += len(data) | ||
| yield data | ||
| chunk_start = chunk_end |
There was a problem hiding this comment.
This assumes that the fetch call above always returns the full chunk, no? I.e., that bytes_received == chunk_end - chunk_start.
There was a problem hiding this comment.
I reworked this a bit, but the general logic here is: we trust that urllib does what we asked it or throws an error. In the event of an incomplete read, we expect to hear about that...
| except urllib.error.HTTPError as exc: | ||
| # S3 returns 403 for missing objects in some bucket configurations | ||
| if exc.code in (403, 404): | ||
| break |
There was a problem hiding this comment.
Does this break the while loop or just the try?
There was a problem hiding this comment.
break doesn't interact with try in any way...
|
|
||
| time.sleep(10) | ||
|
|
||
| # Streaming is over (or never started) — fetch the (remaining) final log |
There was a problem hiding this comment.
So the break above gets us here when .chunks doesn't exist?
mvollmer
left a comment
mvollmer
left a comment
There was a problem hiding this comment.
fetch_log was difficult to follow, but I have no good ideas for how to make it clearer.
Venefilyn
left a comment
Venefilyn
left a comment
There was a problem hiding this comment.
Seems fine but you can also provide log.html and then it just outputs the HTML, which seems unintended. Would be nice for it to take the log.html URL as well and try the log URL itself
That way you can just copy the link from a PR without having to change it
df2e59c to
1e834d2
Compare
tomasmatus
left a comment
tomasmatus
left a comment
There was a problem hiding this comment.
Thanks for the docs! It is much clearer to me how it works now
| # The entire protocol is bytes-oriented: chunk sizes in the manifest are | ||
| # byte counts, Range request offsets are byte offsets, and chunk filenames | ||
| # encode byte ranges. Although character offsets are never used for | ||
| # anything, the content is assumed to be UTF-8 text. The server |
There was a problem hiding this comment.
here you promise UTF-8 in the entire protocol but
self.destination.write('log.chunks', json.dumps(chunk_sizes).encode('ascii')):)
There was a problem hiding this comment.
ascii is utf8 :)
allisonkarlitskaya
left a comment
allisonkarlitskaya
left a comment
There was a problem hiding this comment.
The removal of support for servers without range requests was a mistake because we suggest python3 -m http.server for use with our local log driver and.... it doesn't support range requests.
43a31fe to
aed44c8
Compare
This lets you get the raw log from a complete or in-progress CI run and includes a `--follow` option which exits when the job completes.
s3streamer is effectively a mini protocol that a client and a server speak over S3. There are a few subtle points about the implementation that are important to get straight, so spell them out explicitly. Claude wrote a lot of the text here, but with a great deal of guidance and manual editing. Change to the SPDX header format while we're at it.
On the basis of the origin of the strings in question, it's probably currently impossible for the `data: str` that we pass to s3streamer to contain surrogates, but if it does, `.encode()` will explode. Let's be a bit more gentle about this and replace with `�`.
aed44c8 to
a405927
Compare
4 participants
This lets you get the raw log from a complete or in-progress CI run and includes a
--followoption which exits when the job completes.