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
Enhance logging when inotify beacon is missing pyinotify #60402
Enhance logging when inotify beacon is missing pyinotify #60402
Conversation
This does change the behavior for only this single beacon and not the others. I'm afraid by changing behavior on only one individual beacon, users would expect the same behavior on the other beacons. also @s0undt3ch can you explain why this would be okay to log since its an engine. I initially shared the same concern as you, that this could be noisy and log an error unintentionally for some users. |
True. Update all others on this PR? |
Loaded modules/states get loaded over and over, so, logging on these would cause a lot of log messages. |
Yeah, I think we need to update all beacons on this PR to ensure the experience is the same for all beacons. Is this something you are willing to do @meaksh ? The changelog type should also be |
@meaksh Just pinging on this - were you still planning to add the log change for the other engines? |
0c17565
to
3f7d23c
Compare
@meaksh awesome, thanks! I'll take a look probably next week! |
Hi! I'm your friendly PR bot!You might be wondering what I'm doing commenting here on your PR. Yes, as a matter of fact, I am... I'm just here to help us improve the documentation. I can't respond to Okay... so what do you do? I detect modules that are missing docstrings or "CLI Example" on existing docstrings! So what does that have to do with my PR? I noticed that in this PR there are some files changed that have some of these Okay, what are they? Well, my favorite, is that since you were making changes here I'm hoping that If I can, then what? Well, you can either add them to this PR or add them to another PR. Either way is fine! Well... what if I can't, or don't want to? That's also fine! We appreciate all contributions to the Salt Project. If you Whatever approach you decide to take, just drop a comment here letting us know! Detected Issues (click me)Check Known Missing Docstrings...........................................Failed - hook id: invoke - duration: 1.11s - exit code: 1 Thanks again! |
@garethgreenaway can we get your review here since your familiar with beacons. |
What does this PR do?
This PR simply enhances the logging for "inotify" beacon when
__virtual__
function is returningFalse
due missingpyinotify
library.NOTE: I'm not adding a unit test for this since it is simply a cosmetic change. Is the test required in this case?
Previous Behavior
Without this PR, in case
pyinotify
is not installed on the system, the logs we see in the minions are like the following:As you can see, there is no hint, even with
log_level: debug
that indicates why the beacon cannot be loaded or processed.New Behavior
Now we see an indication about missing
pyinotify
library, even if not running in debug mode, so the user knows what's happening:Merge requirements satisfied?
Commits signed with GPG?
Yes
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.