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.

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

Option to redirect devserver stdout/stderr to a file #1452

Merged
merged 3 commits into from
May 1, 2024

Conversation

tebeka
Copy link
Contributor

@tebeka tebeka commented Apr 30, 2024

Add a Stdout and Stderr options to DevServerOptions that if it's not nil will redirect the spawned server stdout and stderr to them.

What was changed

Option to redirect devserver stdout/stderr to different streams.

Add a Stdout and Stderr options to DevServerOptions that if it's not nil
will redirect the spawned server stdout and stderr to this file.

Why?

We're embedding the dev server in out app, and when we close it we get huge amount of logs from the underlying dev server.

Checklist

  1. Closes

  2. How was this tested:

Ran the current tests.

  1. Any docs updates needed?

Maybe https://docs.temporal.io/dev-guide/go/testing, not sure.

Add a `StdoutLogFile` option to `DevServerOptions` that if it's not ""
will redirect the spawned server stdout and stderr to this file.
@tebeka tebeka requested a review from a team as a code owner April 30, 2024 07:00
@CLAassistant
Copy link

CLAassistant commented Apr 30, 2024

CLA assistant check
All committers have signed the CLA.

@@ -77,6 +77,8 @@ type DevServerOptions struct {
LogLevel string
// Additional arguments to the dev server.
ExtraArgs []string
// Where to redirect stdout and stderr, if empty they will be redirected to the current process.
StdoutLogFile string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StdoutLogFile string
Stdout io.Writer
Stderr io.Writer

This is a more flexible approach, the caller can provide a file if they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, will update the PR soon-ish. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

This looks good to me, will let @Quinn-With-Two-Ns take a peek

@cretz cretz merged commit d051de6 into temporalio:master May 1, 2024
13 checks passed
@cretz
Copy link
Member

cretz commented May 1, 2024

Thanks!

@tebeka
Copy link
Contributor Author

tebeka commented May 2, 2024

Thanks @cretz ! Can you guestimate when you're going to tag a new version?

@itayd
Copy link

itayd commented May 4, 2024

@Quinn-With-Two-Ns what's the policy around when to cut a new version? Thanks

daabr added a commit to autokitteh/autokitteh that referenced this pull request Jun 18, 2024
No functional change in AK, but:

- New Temporal SDK (ENG-820) - includes Miki's
temporalio/sdk-go#1452 (follow-up in separate
PR)
- Many proto & test changes due to newly-working Buf validation checks
in messages and services (which were silently violated until now)
- Removed usage of `shortuuid` (replaced with a TypeID without a prefix)
- Replaced direct usage of YAML v2 parsing with v3

Follow-up in separate PRs:
- ENG-1024
- ENG-1026
- ENG-1027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants