Skip to content

Conversation

@AvaStancu
Copy link
Contributor

@AvaStancu AvaStancu commented Dec 1, 2022

Solves the Listener part needed for https://github.com/github/c2c-actions-runtime/issues/2065
Worker part to follow

image

@AvaStancu AvaStancu requested a review from a team as a code owner December 1, 2022 17:05
@AvaStancu AvaStancu force-pushed the avastancu/fhammerl/stdout-logging branch from 011bc87 to e02f9ab Compare December 2, 2022 09:15
Console.WriteLine($"# {message}");
Console.WriteLine();
}
else
Copy link
Member

Choose a reason for hiding this comment

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

do we need to update all the Write*() functions in the file to have the else?

Copy link
Member

@TingluoHuang TingluoHuang Dec 5, 2022

Choose a reason for hiding this comment

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

might be weird to change those errors get printed to STDOUT instead STDERR.

Copy link
Contributor

@fhammerl fhammerl Dec 5, 2022

Choose a reason for hiding this comment

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

do we need to update all the Write*() functions in the file to have the else?

only the Write*() that don't have a Trace.* Instead of an else, should we actually Trace these always?

We could also do Trace.Info($"WRITE SECTION # {message}"); and Trace.Info($"WRITE SUCCESS √ {message}"); to keep it consistent with other traces in the other Write*() functions?

Copy link
Member

Choose a reason for hiding this comment

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

I was more focused on the WriteError() parts. 😄

Copy link
Contributor

@fhammerl fhammerl Dec 5, 2022

Choose a reason for hiding this comment

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

might be weird to change those errors get printed to STDOUT instead STDERR.

Good point 🤔
In StdoutTraceListener, we could use Console.Error.WriteLine(message); if TraceEventType is ERROR (or CRITICAL)

I think kubectl logs [pod] gives a combined stdout/stderr stream, so it should work for us

@fhammerl fhammerl force-pushed the avastancu/fhammerl/stdout-logging branch from 8e09417 to 3e14982 Compare December 6, 2022 13:50
@fhammerl fhammerl merged commit 088981a into main Dec 6, 2022
@fhammerl fhammerl deleted the avastancu/fhammerl/stdout-logging branch December 6, 2022 15:16
nikola-jokic pushed a commit to nikola-jokic/runner that referenced this pull request May 12, 2023
* Added env variable to control wether the terminal is silent

* Log to stdout if PrintLogToStdout is enabled

* Extracted console logging to stdouttracelistener

* Remove useless usings

* Rewrite TraceListener as superclass

* Only print to stdout if env is set

* Add comment for Console.Out

* Format Listener

* Revert var name in terminal

* Check env in hostcontext instead of Tracing constructor

* Remove superclass & dupe logging code

* Log hostType

* Readonly '_' prefix 'hostType'

* Fix test

* Revert Terminal change

Co-authored-by: Ferenc Hammerl <[email protected]>
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.

4 participants