-
Notifications
You must be signed in to change notification settings - Fork 0
CSTACKEX-34: Upgrade to framework classes design #13
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
Conversation
| * | ||
| * @param values | ||
| */ | ||
| public void updateStorageVolume(Map<String,String> values) |
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.
We can have model where we have nested hierarchy of the file and volume model combined instead of map. This will help us know what fields are required for what operation from readability perspective.
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.
took care
|
|
||
| private static final Logger s_logger = (Logger) LogManager.getLogger(StorageStrategy.class); | ||
|
|
||
| protected enum PROTOCOLS |
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.
OntapStorage also has protocol, we can remove this
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.
took care
| * createNameSpace for Nvme/TCP and Nvme/FC protocol | ||
| * @param values | ||
| */ | ||
| abstract public CloudStackVolume createCloudStackVolume(Map<String,String> values); |
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.
as per discussion, we should pass those new models as a param also instead of map in required methods
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.
took care
|
|
||
| public class AccessGroup { | ||
|
|
||
| Igroup igroup; |
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.
please specify as private because we already have a getter with public
|
|
||
| public class CloudStackVolume { | ||
|
|
||
| FileInfo file; |
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.
same here, define as private
| public class AccessGroup { | ||
|
|
||
| Igroup igroup; | ||
| Policy policy; |
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 should be ExportPolicy object
rajiv-jain-netapp
left a comment
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.
Incorporated review comments
| * | ||
| * @param values | ||
| */ | ||
| public void updateStorageVolume(Map<String,String> values) |
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.
took care
|
|
||
| private static final Logger s_logger = (Logger) LogManager.getLogger(StorageStrategy.class); | ||
|
|
||
| protected enum PROTOCOLS |
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.
took care
| * createNameSpace for Nvme/TCP and Nvme/FC protocol | ||
| * @param values | ||
| */ | ||
| abstract public CloudStackVolume createCloudStackVolume(Map<String,String> values); |
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.
took care
| ISCSI | ||
| } | ||
|
|
||
| public static final String NFS = "nfs"; |
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.
These 2 constants can be deleted
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.
Sure
sandeeplocharla
left a comment
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.
Please see if the constants can be removed as part of the next PR
Description
Updated the class structure for the framwork classes
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?