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

fix(ci): do not keep logging if grep times out or finds the string #7722

Closed
wants to merge 1 commit into from

Conversation

gustavovalverde
Copy link
Member

Motivation

Even though we tried to fix this in #7713, it was still not working on that PR.

Solution

  • Use >(...) which is a form of process substitution in Bash, which avoids the "broken pipe" error as the tee command isn't directly writing to a closed pipe

Review

If added a 5 min timeout here, so if this passes it should be good to go.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@gustavovalverde gustavovalverde added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-High 🔥 I-integration-fail Continuous integration fails, including build and test failures labels Oct 10, 2023
@gustavovalverde gustavovalverde requested a review from a team as a code owner October 10, 2023 10:45
@gustavovalverde gustavovalverde self-assigned this Oct 10, 2023
@gustavovalverde gustavovalverde requested review from upbqdn and removed request for a team October 10, 2023 10:45
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 10, 2023
@gustavovalverde gustavovalverde changed the title fix(ci): do not keep logging if grep timeouts or ends fix(ci): do not keep logging if grep times out or finds the string Oct 10, 2023
@gustavovalverde gustavovalverde marked this pull request as draft October 10, 2023 22:07
@gustavovalverde
Copy link
Member Author

I changed this back to Draft as it's still not working.

I'll be testing a new approach locally and in CI using other Docker options for logging

@teor2345
Copy link
Contributor

If we install ripgrep on our Google Cloud machine, we could use rg --passthru ... rather than tee ... | grep ....

rg accepts the same arguments as grep.

@upbqdn upbqdn added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Oct 13, 2023
@mpguerra
Copy link
Contributor

@gustavovalverde do we still need this one?

@teor2345 teor2345 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-integration-fail Continuous integration fails, including build and test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants