asn, fake expert: default paths, asn: add checks #2606
Conversation
sebix
force-pushed
the
sane-default-path
branch
2 times, most recently
from
bf38b18 to
905a62e
Compare
|
rebased to fix the tests |
| @@ -40,6 +41,8 @@ def init(self): | |||
| self.logger.error("Read 'bots/experts/asn_lookup/README' and " | |||
| "follow the procedure.") | |||
| self.stop() | |||
| if not Path(self.database).is_file: | |||
There was a problem hiding this comment.
An idea - maybe add also a check in the check method that would verify if the file is not a file or is not writable (also check from L121)? Not existing file should not cause a failure in check (it could be like this for a new bot), but could be an info
There was a problem hiding this comment.
The line is actually wrong. is_file is a method, not a member, therefore is_file always evaluates to true.
An idea - maybe add also a check in the check method that would verify if the file is not a file or is not writable (also check from L121)?
I updated the existence check and added a type check
Not existing file should not cause a failure in check (it could be like this for a new bot), but could be an info
I can follow you, but on the other hand, if the database file does not exist at the time of checking, the bot can also not operate with this current state and needs intervention by the admin to get into a working state.
There was a problem hiding this comment.
Ahh, good that you caught it!
I can follow you, but on the other hand, if the database file does not exist at the time of checking, the bot can also not operate with this current state and needs intervention by the admin to get into a working state.
This is also true, so a failure may be expected. I'd anyway keep it more as warning as an error as it could be handled automatically, and is a desired state on the first run - until manual or automated action take place - and the bot configuration itself might be correct.
There was a problem hiding this comment.
I'd anyway keep it more as warning as an error as it could be handled automatically, and is a desired state on the first run - until manual or automated action take place - and the bot configuration itself might be correct.
I disagree with you that the configuration is correct if it's not able to operate.
A better solution, solving your and my concerns, could be to run the auto update method at initialization if the database doesn't exist.
However, I'd like to postpone that feature in favor of getting the changes with improved checks and default values done.
is_file is a method, not a member, therefore is_file always evaluates to true
|
Hehe, the added check already works: I believe the line raising that exception is actually a bug. EDIT: Addressed by #2622 |
asn, fake expert: use state path for database default
asn: Check for database file existence and writability fixes #2566