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.

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

Add support for smart command - slightly different #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jbrekle
Copy link

@jbrekle jbrekle commented Dec 12, 2019

Hi, I made this pull request without noticing there is already a similar one (#15 ). But it seems conflicted and inactive so I open mine anyway.

… health statistics. this feature can be enabled by a new config option called 'smart' allow for the stdout to be logged (so it is part of the report mail).
@jbrekle jbrekle changed the title Add support for smart command - slightly different issue #3 Add support for smart command - slightly different Dec 12, 2019
Copy link
Owner

@Chronial Chronial left a comment

Choose a reason for hiding this comment

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

Thx for the PR. Some minor comments:

"""
Run snapraid command
Raises subprocess.CalledProcessError if errorlevel != 0
"""
stdout_level = logging.INFO if log_output else logging.OUTPUT
stderr_level = logging.ERROR if log_output else logging.OUTERR
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you change the loglevel of stderr here?

@@ -45,11 +45,13 @@ def tee_thread():
return t


def snapraid_command(command, args={}, *, allow_statuscodes=[]):
def snapraid_command(command, args={}, *, allow_statuscodes=[], log_output = False):
Copy link
Owner

Choose a reason for hiding this comment

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

IMO log_ouput is not the right name for this. The output is always logged. It's just that if the user sets the email preference to short, stdout is not included in the email. increase_output_loglevel would be better. Also please follow pep8.

Suggested change
def snapraid_command(command, args={}, *, allow_statuscodes=[], log_output = False):
def snapraid_command(command, args={}, *, allow_statuscodes=[],
increase_output_loglevel=False):

Comment on lines +10 to +11
; prints a SMART report of all the disks of the array.
smart = true
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer for this to be false. Otherwise the script will fail per default if the user doesn't have smartctrl. Also a note Make sure smartctl is in your PATH. in the comment would be helpful.

@Chronial Chronial changed the title issue #3 Add support for smart command - slightly different Add support for smart command - slightly different Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants