Add RSCT validity check in ServiceReport tool #11
Conversation
Signed-off-by: Seeteena Thoufeek <[email protected]>
| from servicereportpkg.utils import execute_command | ||
| from servicereportpkg.check import PackageCheck,ServiceCheck | ||
| from servicereportpkg.utils import is_package_installed | ||
| from servicereportpkg.logger import get_default_logger |
There was a problem hiding this comment.
No one using get_default_logger function anywhere in this file.
Do you really need this function?
| from servicereportpkg.check import Check, SysfsCheck | ||
| from servicereportpkg.validate.plugins import Plugin | ||
| from servicereportpkg.utils import execute_command | ||
| from servicereportpkg.check import PackageCheck,ServiceCheck |
There was a problem hiding this comment.
Format the import statement, need space between PackageCheck and ServiceCheck.
| import os | ||
| import sys | ||
|
|
||
| from servicereportpkg.check import Check, SysfsCheck |
There was a problem hiding this comment.
I think you don't need to import Check and SysfsCheck.
There was a problem hiding this comment.
pylint will help you to find such errors.
There was a problem hiding this comment.
We need to import Check. but SysfsCheck not needed.
| """Checks the subsystem status by issuing the lssrc command""" | ||
|
|
||
| command = ["lssrc", "-s", subsystem] | ||
| (return_code,stdout,err) = execute_command(command) |
There was a problem hiding this comment.
Format issue. Please use pylint to find other similar issues.
| "devices.chrp.base.ServiceRM","DynamicRM"] | ||
| self.subsystems = ["ctrmc","IBM.DRM","IBM.HostRM","IBM.ServiceRM", | ||
| "IBM.MgmtDomainRM"] | ||
| self.subsystem_active = "active" |
There was a problem hiding this comment.
Do we really need to keep a class-level variable here? Can't we use the "active" string directly in get_subsystem_status function?
| status = False | ||
|
|
||
| return PackageCheck(self.check_rsct_package.__doc__, | ||
| package, status) |
There was a problem hiding this comment.
Although you are checking all the packages defined in the self.packages list, but only the last package in the list is added to PackageCheck object.
Not sure whether this is by intention?
There was a problem hiding this comment.
ha. The time when validate code is posted, I did not think we need all those values needs to populate in repair. Now I am returning packagelist along with their status.
| status = False | ||
|
|
||
| return ServiceCheck(self.check_rsct_subsystem_check.__doc__, | ||
| subsystem, status) |
There was a problem hiding this comment.
As we discussed offline you might need to update this function to store all the subsystems along with their status?
There was a problem hiding this comment.
Yes. rsct repair plugin needs these subsystems along with their status so will update the revised code.
| Plugin.__init__(self) | ||
| self.name = RSCT.__name__ | ||
| self.description = RSCT.__doc__ | ||
| self.service_name = "ctrmc" |
There was a problem hiding this comment.
I think we don't need this variable anymore, right?
There was a problem hiding this comment.
Yes. not needed. will remove it.
| command = ["lssrc", "-s", subsystem] | ||
| (return_code,stdout,err) = execute_command(command) | ||
|
|
||
| if return_code is None or self.subsystem_active not in str(stdout): |
There was a problem hiding this comment.
No issue in hardcoding "active" string instead of self.subsystem_active.
| status = True | ||
| self.log.info("RSCT Package check") | ||
| for package in self.packages: | ||
| if not is_package_installed(package): |
There was a problem hiding this comment.
Please use four spaces to align you code. No tabs.
There was a problem hiding this comment.
I am using python formater to align the space. Is that okay?
There was a problem hiding this comment.
installed pylint and fixed the format error.
Signed-off-by: Seeteena Thoufeek <[email protected]>
Conflicts: servicereportpkg/validate/plugins/rsct.py
Signed-off-by: Seeteena Thoufeek [email protected]