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

run gather in background #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SeanMooney
Copy link

run_bg can only be used to run executables in the backgorund
not bash functions. This change adds a wait_for_bg_slot funciton
and refactors the sos report gathering to run in parallel up to
the concurrancy limit.

@openshift-ci openshift-ci bot requested review from abays and Akrog June 17, 2024 17:40
Copy link

openshift-ci bot commented Jun 17, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fmount for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

collection-scripts/bg.sh Outdated Show resolved Hide resolved
run_bg can only be used to run executables in the backgorund
not bash functions. This change adds a wait_for_bg_slot funciton
and refactors the sos report gathering to run in parallel up to
the concurrancy limit.
Copy link

@bogdando bogdando left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@Akrog Akrog left a comment

Choose a reason for hiding this comment

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

I think it would be good to have "why" the run_bg function is not appropriate in the commit message.

@openshift-ci openshift-ci bot removed the lgtm label Jun 19, 2024
@gibizer
Copy link
Contributor

gibizer commented Jun 20, 2024

I've opened a tracker Jira for this https://issues.redhat.com/browse/OSPRH-7830
The bug only appears if the strict error checking is enabled in bash, but we cannot enable that globally in the scripts due to other error handling shortcomings.

@Akrog
Copy link
Contributor

Akrog commented Jun 20, 2024

Thanks @gibizer I think that context is needed in the commit message and the PR description.
From the existing content I thought there was a bad behavior of the script, which doesn't seem to be the case.
Though I agree with the intent of the PR to make changes to enable strict error checking.

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