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

feat: add step name to logger field #1027

Merged
merged 4 commits into from
Mar 14, 2022
Merged

feat: add step name to logger field #1027

merged 4 commits into from
Mar 14, 2022

Conversation

KnisterPeter
Copy link
Member

This change does add the step name to the logger fields. This does
not change the output for our users, but for the json logger, it
does make each step output traceable.

This change does add the step name to the logger fields. This does
not change the output for our users, but for the json logger, it
does make each step output traceable.
@KnisterPeter KnisterPeter self-assigned this Mar 3, 2022
@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #1027 (002f989) into master (4f8da0a) will increase coverage by 1.26%.
The diff coverage is 80.55%.

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   57.50%   58.77%   +1.26%     
==========================================
  Files          32       34       +2     
  Lines        4594     4655      +61     
==========================================
+ Hits         2642     2736      +94     
+ Misses       1729     1690      -39     
- Partials      223      229       +6     
Impacted Files Coverage Δ
pkg/model/action.go 0.00% <ø> (ø)
pkg/model/planner.go 50.73% <ø> (+0.32%) ⬆️
pkg/exprparser/interpreter.go 74.39% <66.66%> (+0.99%) ⬆️
pkg/runner/logger.go 62.85% <67.56%> (-2.58%) ⬇️
pkg/runner/runner.go 74.52% <75.00%> (-1.95%) ⬇️
pkg/runner/expression.go 89.36% <77.61%> (-1.46%) ⬇️
pkg/runner/job_executor.go 82.50% <82.50%> (ø)
pkg/runner/step_context.go 85.81% <84.00%> (+4.17%) ⬆️
pkg/runner/action.go 84.21% <84.21%> (ø)
pkg/runner/run_context.go 80.03% <96.15%> (+0.38%) ⬆️
... and 9 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@KnisterPeter KnisterPeter marked this pull request as ready for review March 3, 2022 13:35
@KnisterPeter KnisterPeter requested a review from a team as a code owner March 3, 2022 13:35
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

This doesn't seem to work for me.
Also every step no longer prints via the job logger.

pkg/runner/logger.go Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team March 5, 2022 16:13
@KnisterPeter
Copy link
Member Author

This doesn't seem to work for me.
Also every step no longer prints via the job logger.

Right, the step should log through the step-logger.
Do you use a different job-logger?

@ChristopherHX
Copy link
Contributor

Do you use a different job-logger?

Yes I use a custom one, since I send the log to github actions service no logging to stdout.

@KnisterPeter
Copy link
Member Author

So, the problem is that a different logger is used. Can't you set the output on the root logger? That should then be used by all context loggers and redirect as you wish

@ChristopherHX
Copy link
Contributor

Can't you set the output on the root logger?

No, this PR overrides everything I set:

stepLogger.SetOutput(os.Stdout)

The other if case is always skipped during debugging of this PR

@KnisterPeter
Copy link
Member Author

Since I don't know your code, I cannot even think of another way to do it. Can you reference your code lines here?

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Mar 5, 2022

Since logrus and context both are immutable for our case, we
can just add a new field and store the logger in the context.
@pull-request-size pull-request-size bot added size/S and removed size/M labels Mar 7, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 7, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Mar 7, 2022
ChristopherHX
ChristopherHX previously approved these changes Mar 8, 2022
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

All fine now, I can confirm that you no longer notice the change without a json logger or a debugger.

@mergify mergify bot removed the needs-work Extra attention is needed label Mar 8, 2022
ZauberNerd
ZauberNerd previously approved these changes Mar 9, 2022
cplee
cplee previously approved these changes Mar 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2022

@KnisterPeter this pull request is now in conflict 😩

@mergify mergify bot added the conflict PR has conflicts label Mar 14, 2022
@ZauberNerd ZauberNerd dismissed stale reviews from cplee, ChristopherHX, and themself via 002f989 March 14, 2022 16:44
@mergify mergify bot removed the conflict PR has conflicts label Mar 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added needs-work Extra attention is needed and removed needs-work Extra attention is needed labels Mar 14, 2022
@cplee cplee merged commit 6b05957 into nektos:master Mar 14, 2022
@KnisterPeter KnisterPeter deleted the log-step-name-field branch March 28, 2022 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants