-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make the error message more clear in case a path is not accessible #2369
Conversation
if not ignore_missing: | ||
raise ConfigurationError(msg) | ||
|
||
self.log.info(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info
or warning
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, let's raise the log level at this point since permission errors will be eventually possible
Codecov Report
@@ Coverage Diff @@
## master #2369 +/- ##
==========================================
+ Coverage 79.56% 89.05% +9.49%
==========================================
Files 204 5 -199
Lines 13777 201 -13576
Branches 1460 35 -1425
==========================================
- Hits 10961 179 -10782
+ Misses 2428 18 -2410
+ Partials 388 4 -384 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing this! Left a small unrelated question.
@@ -8,6 +8,7 @@ | |||
|
|||
from datadog_checks.checks import AgentCheck | |||
from datadog_checks.config import is_affirmative | |||
from datadog_checks.errors import ConfigurationError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this exception only exists since #2367, shouldn't this be reflected on the version constraint of this integration on the datadog-checks-base
wheel? (just asking, not sure what the policy is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still transitioning and very few of the checks pin the base check version, directory
is one of those not pinning. At this moment there's not a real policy, we help users who need to update to manually setup the right set of packages in the agent on a case by case policy. This will change as soon as we outline the Agent CLI requirements for the single check update.
What does this PR do?
On Windows,
os.path.exists
returnsFalse
both when a directory doesn't exist and when the user can't access it for lack of permissions, we now mention this in the error message.Motivation
Since upcoming version of the Agent will run under an unprivileged user, a permission error will be possible in the future, let's be proactive.
Review checklist
no-changelog
label attachedAdditional Notes
Logic doesn't change, it's just the error message