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.

Sign up for GitHub

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

Jump to bottom

fix: do not log .conf parser warnings from all workers #845

Merged
merged 5 commits into from
May 29, 2024
Merged

fix: do not log .conf parser warnings from all workers #845

merged 5 commits into from
May 29, 2024

Conversation

harshilgajera-crest
Copy link
Contributor

@harshilgajera-crest harshilgajera-crest commented May 28, 2024 •
edited
Loading

When tests are ran with multiple workers, duplicate logging is observed because every workers logs the warning.
This PR fixes that.

@harshilgajera-crest harshilgajera-crest changed the base branch from main to develop May 28, 2024 05:41
@harshilgajera-crest harshilgajera-crest changed the title Fix/logging issue fix: logging issue May 28, 2024
@harshilgajera-crest harshilgajera-crest marked this pull request as ready for review May 28, 2024 07:43
@harshilgajera-crest harshilgajera-crest requested a review from a team as a code owner May 28, 2024 07:43
artemrys
artemrys reviewed May 28, 2024
View reviewed changes
Copy link
Member

@artemrys artemrys left a comment

Choose a reason for hiding this comment

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

Please extract this logic in a helper function, it's been used several times already across the codebase.

@harshilgajera-crest
Copy link
Contributor Author

Please extract this logic in a helper function, it's been used several times already across the codebase.

Done

mkolasinski-splunk
mkolasinski-splunk reviewed May 29, 2024
View reviewed changes
Copy link
Contributor

@mkolasinski-splunk mkolasinski-splunk left a comment

Choose a reason for hiding this comment

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

I'd consider creating separate utils file with check_first_worker() instead of having delayed imports. @artemrys wdyt?

@harshilgajera-crest
Copy link
Contributor Author

I'd consider creating separate utils file with check_first_worker() instead of having delayed imports. @artemrys wdyt?

I thought the same but because it was only one small function i thought it was not needed, but we can do that.

@artemrys artemrys changed the title fix: logging issue fix: do not log .conf parser warnings from all workers May 29, 2024
@artemrys artemrys merged commit 8d4fb44 into develop May 29, 2024
32 of 34 checks passed
@artemrys artemrys deleted the fix/logging-issue branch May 29, 2024 10:24
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
@srv-rr-github-token
Copy link
Contributor

🎉 This PR is included in version 5.3.0-beta.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@srv-rr-github-token
Copy link
Contributor

🎉 This issue has been resolved in version 5.4.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@artemrys artemrys artemrys approved these changes

@kdoroszko-splunk kdoroszko-splunk Awaiting requested review from kdoroszko-splunk

@mkolasinski-splunk mkolasinski-splunk Awaiting requested review from mkolasinski-splunk

Assignees
No one assigned
Labels
released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harshilgajera-crest @srv-rr-github-token @artemrys @mkolasinski-splunk