Conversation
add Eternus mapping
edit list_masking_views func
Codecov Report
@@ Coverage Diff @@
## master #821 +/- ##
==========================================
+ Coverage 70.87% 71.32% +0.44%
==========================================
Files 182 182
Lines 20384 20823 +439
Branches 3087 3177 +90
==========================================
+ Hits 14448 14852 +404
- Misses 4953 4966 +13
- Partials 983 1005 +22
|
edit list_masking_views func
list_masking_views func efficiency optimization
There was a problem hiding this comment.
The host_fc_list returned by self.get_data already contains an empty list. You do not need to add [].
There was a problem hiding this comment.
Has been modified
There was a problem hiding this comment.
The consts.HOST_TOTAL has been transferred in the self.get_data method. Why does it need to be checked?
There was a problem hiding this comment.
I deleted the first validation, but the two previous validations were done for different data sources
| def get_access_url(): | ||
| return 'https://{ip}' | ||
|
|
||
| def list_storage_host_initiators(self, ctx): |
There was a problem hiding this comment.
The function is too long. You are advised to extract functions for each type of processing.
There was a problem hiding this comment.
Duplicate code. The structure is the same but the parameter values are different. The method should be extracted and transferred as input parameters.
| iscsi_name = host_iscsi.get('name') | ||
| state = host_status.get(iscsi_name) | ||
| os = host_iscsi.get('os') | ||
| os = os.lower() if os else None |
There was a problem hiding this comment.
It is recommended that this operation be processed in self.get_iscsi_host() and then returned.
There was a problem hiding this comment.
get_iscsi_host is a public function. List_storage_host_initiators and list_storage_hosts are both used. If you put this code after get_iscsi_host,The function will no longer be a public function, resulting in duplicate code
There was a problem hiding this comment.
The return value of the split() method is a list, and no additional judgment of or [] is required.
There was a problem hiding this comment.
Has been modified
| for status_row in status_list: | ||
| if len(status_row) < consts.HOST_PATH_STATUS_TOTAL: | ||
| continue | ||
| host_name = status_row[consts.HOST_PATH_STATUS_NAME] |
There was a problem hiding this comment.
The two variables are used only once, and the constant consts.HOST_PATH_STATUS_NAME can be read. Therefore, you do not need to define the two variables separately. You need to delete the two variables.
There was a problem hiding this comment.
I only used it once, but you can't use Arabic numerals directly in a function because it's a very common behavior, so you can think about it a little bit
There was a problem hiding this comment.
Storage_id is not changed in later use. You do not need to define a variable to reference it. You can directly use self.storage_id.
There was a problem hiding this comment.
Has been modified
There was a problem hiding this comment.
If ctx is not used, set this parameter to None. Otherwise, use from delfin.context import RequestContext instead.
There was a problem hiding this comment.
Has been modified
There was a problem hiding this comment.
The assert should compare a complete piece of data rather than a field value. Otherwise, the unit test is useless.
There was a problem hiding this comment.
Has been modified
There was a problem hiding this comment.
It is recommended that the code be placed in the next line for better readability.
There was a problem hiding this comment.
Has been modified
There was a problem hiding this comment.
These three sentences can be combined using a list derivation expression, which is more efficient.
length_list.extend([len(identify) for identify in host_row_str.split()])
There was a problem hiding this comment.
Has been modified
| "storage_id": self.storage_id, | ||
| "native_storage_host_id": fc_name, | ||
| "os_type": consts.HOST_OS_TYPES_MAP.get( | ||
| os, constants.HostOSTypes.UNKNOWN), |
There was a problem hiding this comment.
Format it, there should be a hierarchy.
There was a problem hiding this comment.
Has been modified
codeview modify
add eternus os_type
7 participants
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: