Conversation
|
@Chronial anything I need to do to close out this MR? |
Chronial
left a comment
Chronial
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review this and thx for the reminder :).
snapraid-runner.py
Outdated
|
|
||
| config["smtp"]["ssl"] = (config["smtp"]["ssl"].lower() == "true") | ||
| config["scrub"]["enabled"] = (config["scrub"]["enabled"].lower() == "true") | ||
| config["smart"]["enabled"] = (config["smart"]["enabled"].lower() == "true") |
There was a problem hiding this comment.
Is this save, or will this raise an exception if the smart section is missing from the config? I don't remember ^^.
There was a problem hiding this comment.
I'll test & confirm what happens if the config omits this section and design a method to handle if it raises an exception.
There was a problem hiding this comment.
Confirmed it does not throw an error with latest code on Windows & Linux.
snapraid-runner.py
Outdated
| if config["smart"]["enabled"]: | ||
| logging.info("Running SMART...") | ||
| # Expand PATH to include /usr/sbin for running smartctl from cron | ||
| os.environ["PATH"] += os.pathsep + "/usr/sbin" |
There was a problem hiding this comment.
This is clearly not cross-platform :). Even on unix it makes strong assumptions about the user's setup. I assume you had problems with the binary not being found? Why don't you make the binary path a setting?
There was a problem hiding this comment.
Good point, I definitely overlooked the fact that snapraid is cross-platform and wrote this for Linux.
There was a problem hiding this comment.
You should probably remove this and just make sure the command is run in the correct environment.
There was a problem hiding this comment.
I handled this with known paths in my latest commit. What do you think of the changes to make it cross-platform?
There was a problem hiding this comment.
It look very comprehensive, but I don't think this script should make any attempt to find the executable. That is just very complicated and the PATH system of the OSs is perfectly suitable to handle this job. I will have a look and do some changes to this PR.
snapraid-runner.py
Outdated
| else: | ||
| threads = [ | ||
| tee_log(p.stdout, out, logging.OUTPUT), | ||
| tee_log(p.stderr, [], logging.OUTERR)] |
There was a problem hiding this comment.
Which output belongs to which command is always clearly identifiable from the headers. Why does smart need this logging trickery?
There was a problem hiding this comment.
I created a new SMART logging level that is output in both "short" and "long" logging modes since it is highly likely that it is always desired to view the SMART output if you are sending emails on success. It does not output if set to only error notification emails since the log level is set one level higher than error logging. This section ensures the SMART "OUTPUT" logging is redirected into "SMART" logging while leaving all other logging as-is.
There was a problem hiding this comment.
Ah, ok – that makes sense. Please add a comment about that to where you define that loglevel.
Also add an output_log_level parameter to this function and use that instead of this if.
There was a problem hiding this comment.
I added a comment on line 175 to highlight the SMART logging level creation, but I'm not sure what you're looking for in the output_log_level instead of this if
There was a problem hiding this comment.
Thanks for the changes. I meant that the signature should be snapraid_command(command, args={}, ignore_errors=False, output_log_level=logging.OUTPUT) and then use the parameter instead of guessing based on the name of the command.
|
Thanks for working on this! 🎉 |
Chronial
changed the title
smart commandsmart command
3 participants
The formatting output in the email isn't the greatest, but it's enough to discern the result of the SMART command.