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

tink-worker: do not attach the action container stdout/stderr to the current process when --capture-action-logs=false #549

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

rgl
Copy link
Contributor

@rgl rgl commented Oct 9, 2021

Description

when tink-worker is configured with --capture-action-logs=false the action
container stdout/stderr must not be attached to the current process.

that configuration is used when there is an existing docker log-driver, which
will capture all the containers logs without tink-worker help.

Why is this needed

this prevents the action container logs from being duplicated in the
tink-worker container logs and in the action container docker logs.

How Has This Been Tested?

I've manually tested this locally.

How are existing users impacted? What migration steps/scripts do we need?

No impact is expected.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

when tink-worker is configured with --capture-action-logs=false the action
container stdout/stderr must not be attached to the current process.

that configuration is used when there is an existing docker log-driver, which
will capture all the containers logs without tink-worker help.

this prevents the action container logs from being duplicated in the
tink-worker container logs and in the action container docker logs.

Signed-off-by: Rui Lopes <[email protected]>
@rgl rgl force-pushed the rgl-fix-capture-actions-logs branch from 0404155 to c362740 Compare October 9, 2021 19:41
@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #549 (c6ce199) into main (948f687) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #549   +/-   ##
=======================================
  Coverage   34.76%   34.76%           
=======================================
  Files          44       44           
  Lines        3348     3348           
=======================================
  Hits         1164     1164           
  Misses       2085     2085           
  Partials       99       99           
Impacted Files Coverage Δ
cmd/tink-worker/internal/action.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 948f687...c6ce199. Read the comment docs.

@nshalman
Copy link
Member

nshalman commented Oct 19, 2021

This seems reasonable to me, but I don't have all the context. The branch needs to be updated too.

@nshalman
Copy link
Member

nshalman commented Nov 2, 2021

This seems reasonable to me, but I don't have all the context. The branch needs to be updated too.

@rgl are you still interested in working on this?

@rgl
Copy link
Contributor Author

rgl commented Nov 2, 2021

@nshalman, is there anything else needed in this PR?

@nshalman
Copy link
Member

nshalman commented Nov 2, 2021

branch needs to be updated too

I guess I could theoretically update the branch for you.

@nshalman nshalman added the ready-to-merge Signal to Mergify to merge the PR. label Nov 2, 2021
@mergify mergify bot merged commit 8208713 into tinkerbell:main Nov 2, 2021
@rgl
Copy link
Contributor Author

rgl commented Nov 2, 2021

@nshalman when there are no conflicts, why is there a need to rebase/merge master into the PR?

@nshalman
Copy link
Member

nshalman commented Nov 2, 2021

@nshalman when there are no conflicts, why is there a need to rebase/merge master into the PR?

Looks like there wasn't. My mistake. Thank you!

@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants