rajiv-jain-netapp commented on PR #13053: URL: https://github.com/apache/cloudstack/pull/13053#issuecomment-4829722386
> > @nvazquez @winterhazel could you please check if your comments have been addressed ? > > please let me know if this PR is good to you. > > The new changes look good to me, and the overall code as well. There's just a few comments that I think could still be addressed here. > > [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#discussion_r3204140918) - comment not matching the constant's value. It would be nice to confirm whether the constant's value is indeed the expected one and correct this comment if so. [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#discussion_r3223364310) - not sure if I was able to express myself correctly on this one, could you have another look @piyush5netapp? I was thinking about removing the `volumeInfo = volFactory.getVolume(volumeInfo.getId(), primaryDataStore)` from there by setting the details (in-memory) inside the plugin's `grantAccess` method to prevent some extra database calls. [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#discussion_r3195873738) - minor remark, no need to create a map here. [#13053 (comment)](https://github.com/apache/cloudstack/pull/13053#discussion_r3195875721) - minor remark, no need to create a map here. We have responded to all the comments, Note: we are planning to raise another PR by EOD today, mostly for fixes and extensions on snapshot workflows -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
