[GitHub] cloudstack issue #1727: CLOUDSTACK-9539: Support changing Service offering f...

[rafaelweingartner]

Github user rafaelweingartner commented on the issue:

    https://github.com/apache/cloudstack/pull/1727
  
    Long time I do not review a PR here. I have to say, @nvazquez , great job!!
    @serg38 you read my mind!! I had just started reading @nvazquez PR 
description when you called me.
    
    Well documented methods. The methods are short and concise. The code is 
clear and looks nice (at least for me).
    
    I jumped over the 70+ messages here. So, excuse me if I ask something that 
has already been clarified before.
    
    However, as always I have something to complain; I might be getting old, 
that is probably why :) . Just teasing; it is more like a set of suggestion;
    
    * On VMSnapshotVO, what about declaring the instance attribute you create 
as private? I know the others are not, and at the end of the day, it does not 
change much. However, as long as we are using Java and if we consider 
“VMSnapshotVO” as a POJO, then it feels a good idea to use private 
attributes that are only accessible to their gets/sets.
    * On VMSnapshotManagerImpl, here we have a “service” layer 
object/component. I would have the same suggestion regarding the use of 
private/protected attributes here (this is cosmetics).
    * Still on VMSnapshotManagerImpl, what about extracting the code from lines 
358-373 to a method? This may enable you to write unit test cases for these 
lines.
    * Also on VMSnapshotManagerImpl, methods 
“addSupportForCustomServiceOffering”, “updateUserVmServiceOffering”, 
“getVmMapDetails”, “changeUserVmServiceOffering”, 
revertUserVmDetailsFromVmSnapshot, and “ugradeUserVmServiceOffering” are 
clean and well documented. However, I am missing their test cases. What about 
some test cases here, then it would be awesome.
    * on VMSnapshotManagerImpl, the method “ugradeUserVmServiceOffering” 
seems to have a typo mistake ;)
    * the method “revertUserVmDetailsFromVmSnapshot” receives a parameter 
“userVm”, but it does not seem to be used.
    
    I am assuming you have executed the functional test 
“test_vm_snapshots.py” and it passed successfully, right?
    
    Very good feature to be added. Thanks for the valuable contribution to ACS 
👍 
    
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---