-
Notifications
You must be signed in to change notification settings - Fork 37
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
Rdfg enhancements #84
Conversation
…ype header incorrectly in lowercase. Added skip to 2 tests in provisioning ci_tests for external wwn.
…ype header incorrectly in lowercase. Added skip to 2 tests in provisioning ci_tests for external wwn.
…on thanks to extra space with pluralisation. :(
…t test and ci_test + update to change log.
csv reader that are used in csv writer. Added skips to effective wwn tests where no effective wwn enabled volumes can be found.
test to prevent issues where the in-use initiator that was selected was in a group causing conflicts when attempting to set flags.
added create_storage_group_from_rdfg ci_test and unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few style issues only. Also, just for future reference, perhaps we separate the issues into pull requests so for example application-type fix and initiator group tests fix get their own pull request. Just a suggestion.
PyU4V/replication.py
Outdated
@@ -11,7 +11,7 @@ | |||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
"""replication.py.""" | |||
'''replication.py.''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc strings should be in double quotes
PyU4V/replication.py
Outdated
|
||
|
||
class ReplicationFunctions(object): | ||
"""ReplicationFunctions.""" | ||
'''ReplicationFunctions.''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they have been changed throughout the document. Please change them to ". Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Helen that was a find replace error.
PyU4V/replication.py
Outdated
Optional boolean parameters to set are "bypass", "metroBias", "star", | ||
"immediate", "hop2", "consExempt", "force", "symForce". | ||
Optional boolean parameters to set are 'bypass', 'metroBias', 'star', | ||
'immediate', 'hop2', 'consExempt', 'force', 'symForce'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to avoid confusion. Having single quotes here is fine. It is also in the doc strings so less concerned with this anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed back to original, this change was made in error
PyU4V/replication.py
Outdated
|
||
:param array_id: 12 digit serial number for PowerMax array -- str | ||
:param director_id: identifier for director e.g. RF-1F -- str | ||
:param filters: optional filters - dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -- instead of -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PyU4V/replication.py
Outdated
:param array_id: 12 digit serial number for PowerMax array -- str | ||
:param director_id: identifier for director e.g. RF-1F -- str | ||
:param port_id: port number -- int | ||
:returns: port details --dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - add a stace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good eyes
PyU4V/tests/ci_tests/base.py
Outdated
remote_rdf_director_list = self.replication.get_rdf_director_list( | ||
filters={'online': True}) | ||
for director in remote_rdf_director_list: | ||
remote_rdf_director_port_list = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brackets instead of \
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PyU4V/tests/ci_tests/base.py
Outdated
"""Helper function to get RDFG on arrays that is free for use | ||
|
||
:returns: next RDF group number on local and remote array | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, found another like this
@@ -11,11 +11,11 @@ | |||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
"""pyu4v_common_data.py.""" | |||
'''pyu4v_common_data.py.''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double quotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stopped reviewing this, too much to add in one review.
Fix the parameters, their ordering, and collapse all the multi-line stuff to as few lines as possible to save on space in the overall file, it helps out IDEs load inspections faster. There is also a lot of trailing ) or } which could be moved up a line or two to further save space.
After these are done ill go through the code for problems
PyU4V/replication.py
Outdated
resource_level=SYMMETRIX, resource_level_id=array_id, | ||
resource_type=RDF_DIRECTOR, resource_type_id=director_id, | ||
resource=PORT, params=filters | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: move the ) to the previous line instead of a line of its own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PyU4V/replication.py
Outdated
resource_level=SYMMETRIX, resource_level_id=array_id, | ||
resource_type=RDF_DIRECTOR, resource_type_id=director_id, | ||
resource=PORT, resource_id=port_id | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move ) to previous line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PyU4V/replication.py
Outdated
"action": "set_label" | ||
} | ||
|
||
elif rdfg_action == 'add_ports' or 'remove_ports': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wont work, second half of or statement will always evaluate to True because it is a populated string. Should be:
elif rdfg_action == 'add_ports' or rdfg_action == 'remove_ports':
A better implementation would be
elif rdfg_action in ['add_ports', 'remove_ports']:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed will need to re-run ci to verify still working
PyU4V/replication.py
Outdated
resource_level_id=array_id, | ||
resource_type=RDF_GROUP, | ||
resource_type_id=srdf_group_number, | ||
payload=payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collapse these into a few lines, save space where possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PyU4V/replication.py
Outdated
resource_level_id=array_id, | ||
resource_type=RDF_GROUP, | ||
resource_type_id=srdf_group_number, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move ) to previous line, collapse params to as few lines as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done and collapsed
csv reader that are used in csv writer. Added skips to effective wwn tests where no effective wwn enabled volumes can be found.
test to prevent issues where the in-use initiator that was selected was in a group causing conflicts when attempting to set flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More changes required there, nothing major its almost all formatting
…take a look at
…ancements # Conflicts: # PyU4V/replication.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good Paul, once the CSVReader delimiter comment is addressed ill give it my +1
be assigned by function parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, all requested changes made and CI has had 4 clean runs since most recent commit.
…rmatting for documentation
…rmatting for documentation, one minor typo and additional content added to create_storage_group_from_rdf_group
…rmatting for documentation, one minor typo and additional content added to create_storage_group_from_rdf_group, configuration.rst -> line 152: arguements should be arguments reccomendations.rst -> line 13: it's should be its support.rst -> line 4: Github should be GitHub tools.rst -> lines 37, 41, 73: Github should be GitHub
Rdfg enhancements, documentation and change log updates
Replication Enhancements
RDF functions added
-create_rdf_group
-modify_rdf_group
-delete_rdf_group
-get_rdf_port_connections
-get_rdf_director_port_details
-get_rdf_director_ports
-get_rdf_director
-get_rdf_director_list
-create_storage_group_from_rdfg
BugFix
update to rest_requests header parameter corrected to application-type line 54
Fix in CI_Test for effective_wwn functions and set initiator flags