Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(dgw): support TRP streaming #1188

Merged
merged 9 commits into from
Jan 14, 2025
Merged

feat(dgw): support TRP streaming #1188

merged 9 commits into from
Jan 14, 2025
+460 −111

Conversation

irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Jan 12, 2025 •
edited
Loading

No description provided.

@github-actions GitHub Actions
Copy link

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

@irvingoujAtDevolution irvingoujAtDevolution changed the title stream trp feat(dgw): support TRP streaming Jan 12, 2025
@irvingoujAtDevolution irvingoujAtDevolution marked this pull request as ready for review January 13, 2025 12:05
CBenoit
CBenoit reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

thought: Maybe ascii-streamer should be term-streamer if we’re going to support more protocols? Is ascii appropriate for both asciinema and trp? Thoughts?

crates/ascii-streamer/examples/decode_trp.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/Cargo.toml Outdated
@@ -5,9 +5,11 @@ edition = "2021"

[dependencies]
anyhow = "1.0"
serde_json = "1.0"
tokio = { version = "1.42", features = ["io-util", "sync"] }
tokio = { version = "1.42", features = ["io-util", "sync", "macros", "rt-multi-thread", "time"] }
Copy link
Member

Choose a reason for hiding this comment

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

question: Can you document why more features are necessary now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in Cargo.toml or here in Github?

Copy link
Member

Choose a reason for hiding this comment

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

It’s better to document at the relevant places as GitHub PRs are not something we can refer to easily.

crates/ascii-streamer/src/lib.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
crates/ascii-streamer/src/trp_decoder.rs Outdated Show resolved Hide resolved
devolutions-gateway/src/streaming.rs Outdated Show resolved Hide resolved
@irvingoujAtDevolution
Copy link
Contributor Author

thought: Maybe ascii-streamer should be term-streamer if we’re going to support more protocols? Is ascii appropriate for both asciinema and trp? Thoughts?

WIth regards to this, the argument could be that the output format is always asciinema v2, so it would not be wired to call this crate ascii-streamer, but yeah, term-streamer is better.

@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Jan 14, 2025 •
edited
Loading

Added dependency tokio macro for
crates\terminal-streamer\src\lib.rs:64:16

Added dependency tokio time for
crates\terminal-streamer\src\trp_decoder.rs:69:24

Added dependency tokio rt-multi-thread for
crates\terminal-streamer\src\trp_decoder.rs:15:23

@CBenoit

CBenoit
CBenoit approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Good work, feel free to merge!

@CBenoit
Copy link
Member

CBenoit commented Jan 14, 2025 •
edited
Loading

Added dependency tokio macro for crates\terminal-streamer\src\lib.rs:64:16

Added dependency tokio time for crates\terminal-streamer\src\trp_decoder.rs:69:24

Added dependency tokio rt-multi-thread for crates\terminal-streamer\src\trp_decoder.rs:15:23

Thank you!
Also see this: #1188 (comment)
But I’ll leave up to you the decision about whether it’s useful to keep the information or not.

@irvingoujAtDevolution irvingoujAtDevolution merged commit 5539ac6 into master Jan 14, 2025
36 checks passed
@irvingoujAtDevolution irvingoujAtDevolution deleted the stream-trp branch January 14, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CBenoit CBenoit CBenoit approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@irvingoujAtDevolution @CBenoit