Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Fixed issue with tail not deserailzing event properly #1994

Merged
merged 5 commits into from
Jul 15, 2021
Merged

Conversation

jspspike
Copy link
Contributor

Also added color to wrangler dev and wrangler tail -f pretty json outputs if deserialized properly
Fixes #1990

@jspspike jspspike requested a review from a team July 13, 2021 20:55
Copy link
Contributor

@caass caass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good change :) one comment and a few nits, and then I think it's ready to go.

Cargo.toml Outdated Show resolved Hide resolved
src/tail/log_server.rs Outdated Show resolved Hide resolved
src/tail/log_server.rs Show resolved Hide resolved
src/terminal/json.rs Outdated Show resolved Hide resolved
src/terminal/json.rs Show resolved Hide resolved
@jspspike jspspike requested a review from caass July 14, 2021 19:38
@jspspike
Copy link
Contributor Author

Depends on chrome-devtools-rs#51 getting merged and released

Copy link
Contributor

@nilslice nilslice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caass -- this would be good to get into the upcoming release. Mind taking a final pass on it and ✅ ?

@nilslice nilslice self-requested a review July 14, 2021 21:29
@nilslice
Copy link
Contributor

oops, missed the failing test.

@jspspike, would you be able to format this (cargo fmt on save is nice), and check for clippy suggestions? I tend to run cargo clippy --all --all-features -- -D warnings before any push.

@jspspike
Copy link
Contributor Author

oops, missed the failing test.

@jspspike, would you be able to format this (cargo fmt on save is nice), and check for clippy suggestions? I tend to run cargo clippy --all --all-features -- -D warnings before any push.

The tests are failing on this because it's using things I've added to chrome-devtools-rs that are not in a release yet. So when compiling it fails, need to have chrome-devtools-rs#51 re-reviewed and then update the Cargo.toml before this can get merged.

@nilslice
Copy link
Contributor

Depends on chrome-devtools-rs#51 getting merged and released

@jspspike hah, that will teach me to read the rest of the comments :)

Copy link
Contributor

@caass caass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do we now need to publish a new version of chrome-devtools-rs? Other than that, we're pretty much good, right?

Cargo.toml Outdated Show resolved Hide resolved
src/tail/log_server.rs Outdated Show resolved Hide resolved
@jspspike jspspike merged commit a854d9c into master Jul 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the josh/tail-log branch July 15, 2021 17:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrangler tail: Error parsing response body!
4 participants