Skip to content
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

Feature/add list virtual machine sizes functionalities #405

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KevinLoiseau
Copy link
Contributor

Hi !

This PR implement the virtual machine size's listing, through request and model.

Thanks in advance for your time

test/test_helper.rb Outdated Show resolved Hide resolved
test/test_helper.rb Outdated Show resolved Hide resolved
test/api_stub/models/compute/virtual_machine_size.rb Outdated Show resolved Hide resolved
test/api_stub/models/compute/virtual_machine_size.rb Outdated Show resolved Hide resolved
test/api_stub/models/compute/virtual_machine_size.rb Outdated Show resolved Hide resolved
@KevinLoiseau KevinLoiseau force-pushed the feature/add_list_virtual_machine_sizes_functionalitie branch from f52c14b to b7680e6 Compare October 30, 2018 09:29
@KevinLoiseau KevinLoiseau force-pushed the feature/add_list_virtual_machine_sizes_functionalitie branch from b7680e6 to c9734ff Compare October 30, 2018 09:31
@KevinLoiseau KevinLoiseau force-pushed the feature/add_list_virtual_machine_sizes_functionalitie branch 2 times, most recently from da0f51b to ba5f291 Compare October 30, 2018 13:29
@KevinLoiseau KevinLoiseau changed the title Feature/add list virtual machine sizes functionalitie Feature/add list virtual machine sizes functionalities Oct 30, 2018
@maham-nazir-confiz
Copy link
Contributor

Hello @KevinLoiseau
I will be reviewing and testing this PR. I will merge it if it passes all checks. It might take me some time. Thank you for your patience.

@maham-nazir-confiz
Copy link
Contributor

Hi @KevinLoiseau

I have reviewed your PR. The following changes are requested

  • Incorporate HoundCI feedback if possible.
  • Add the memory_in_mb attribute in the virtual_machine_size model as well.
  • Incorporate the feedback I have added in the code.

Thank you.
Regards

@KevinLoiseau KevinLoiseau force-pushed the feature/add_list_virtual_machine_sizes_functionalitie branch from ba5f291 to d693959 Compare November 13, 2018 13:43
@KevinLoiseau
Copy link
Contributor Author

Hi @maham-nazir-confiz

Thank you for your feedbacks, especially for memory_in_mb, thanks for catching this.
Your feedbacks are implemented.

Regards,
Kevin Loiseau

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau
Thank you for incorporating my feedback. I have a couple more changes to ask of you.

  • Please write an integration test for this as well
  • Can you tell me how did you test the list method with async = true? Because if you change async to true and then call the list method via the collection::all method, it throws the following exception
    Integration Test for virtual machine is failing! #<NoMethodError: undefined method 'map' for #<Concurrent::Promise:0x0055a658124bd0>>
    Please get back to me on this

Regards

@KevinLoiseau KevinLoiseau force-pushed the feature/add_list_virtual_machine_sizes_functionalitie branch from e4ffd88 to 0285368 Compare December 30, 2018 20:42
@KevinLoiseau
Copy link
Contributor Author

KevinLoiseau commented Dec 30, 2018

@maham-nazir-confiz I added an integration test for this part.
For your question about async = true, I tested it directly through the request because through the collection::all that could not works as the response object class is Concurrent::Promise and doesn't respond to map.
For the test; I called the request list_virtual_machine_sizes with async = true, i get an object Concurrent::Promise that is, after few instant, completed with the expected values (the list of virtual machine sizes).

Regards

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau
If you look at all the list methods that we have in fog-azure-rm you'll see that we have not used the async attribute because they all need to be parsed by the Collection::all method into their respective models.

You should update the PR to not use the async attribute at all and also use the list_as_lazy method as we have used in all the list request methods.

Regards,
Maham Nazir

@KevinLoiseau
Copy link
Contributor Author

@maham-nazir-confiz
list_as_lazy is not implemented for virtual_machine_sizes (as the result list is finit on provider side, it's not a problem to provide by default all assets).

For the async call, I agree that we can't use it in Collection::all, but as it's implemented by Azure IMHO it's better to have it in the request part, the model stack will just not implement it by this way, it's not a problem.

If you really want I can remove the async call, but I don't think that is an added value for the project. WDYT ?

Regards,
Kevin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants