Add a new instance action: Resize #29
Conversation
| @@ -51,6 +52,14 @@ def __init__(self, *args, **kwargs): | |||
| self.request_user = user | |||
| super(InstanceSerializer, self).__init__(*args, **kwargs) | |||
|
|
|||
| def get_size(self, obj): | |||
| from api.v2.serializers.summaries import ( | |||
There was a problem hiding this comment.
@kamfonik hmm, I will ask you on this then. Since this is only used by this function. Sometimes I see people import them within only the functions. But we could also put them on top. What do you think, Laura?
There was a problem hiding this comment.
PEP8 recommends imports only at the top of the file, but I can see that there are already function-level imports elsewhere in this code.
From what I have read there may be a slight performance cost to importing inside the function, but probably only on the first call to the function (after that the module should be cached).
In discussions about this on StackOverflow there were a few notes that some people do import within the function while developing, until they have some stable code and know what they need and where it is used, then they move all the imports to the top to comply with PEP8.
Mostly my thought is that if we are going to violate PEP8 there should be a reason, although the reason could be simple like "consistency with existing code" etc.
There was a problem hiding this comment.
@kamfonik Thanks for researching on this. I could clean up the functions that I have modified/created. As for other elsewhere in code base. We either need to have a new PR for cleaning all the code, or keep ignoring them, but make sure we do it properly for the furture PRs.
There was a problem hiding this comment.
@xuhang57 sure, cleaning up is probably not urgent, but deciding now what you want for new code will save a lot of work later.
There was a problem hiding this comment.
Yes it should be moved to the top
| esh_driver.__class__, esh_driver.provider, | ||
| esh_driver.identity, esh_instance.id, | ||
| core_identity.provider.id, core_identity.id, core_identity.created_by) | ||
| task_four = deploy_init_to.si( |
There was a problem hiding this comment.
since you are removing task_four, can we remove tthe import of deploy_init_to above in line 361?
| @@ -51,6 +52,14 @@ def __init__(self, *args, **kwargs): | |||
| self.request_user = user | |||
| super(InstanceSerializer, self).__init__(*args, **kwargs) | |||
|
|
|||
| def get_size(self, obj): | |||
There was a problem hiding this comment.
I can't find where this new function comes into play. Does it get called in some way by your frontend code?
There was a problem hiding this comment.
@kamfonik Yes, so get_size gets called when we havesize = SerializerMethodField (Line 26) it will call def get_<field-name>
Description
API change for allowing a new instance action: Resize.
Checklist before merging Pull Requests
Related PR(s)
The front-end change PR: 11
Potential Problem
We may don't want to allow any user to resize from a larger flavor to a smaller one. OpenStack Horizon allows the user to do that and it might cause problems.
Will discuss this in more details.
Thanks
Lucas