Overwriting the methods retrieve! and exists? in the volume #274

aalexmonteiro · 2017-09-26T21:02:55Z

Description

Overwriting the methods retrieve! and exists? in the volume.

Issues Resolved

#273

Check List

[] New functionality includes testing.
- All tests pass ($ rake test).
New functionality has been documented in the README if applicable.
- New functionality has been thoroughly documented in the examples (please include helpful comments).
Changes are documented in the CHANGELOG.

fgbulsoni · 2017-09-27T12:23:49Z

CHANGELOG.md

+## v5.0.5(unreleased)
+
+#### Bug fixes & Enhancements
+- [#273](https://github.com/HewlettPackard/oneview-sdk-ruby/issues/273) Find a volume using the retrieve! and exists? methods on the API500 before a post


Perhaps something in the lines of: 'retrieve!' and 'exists?' methods return false for existing Volumes on API500 ?
The issue title as it is now might not be super clear.

ricardogpsf · 2017-09-27T14:03:04Z

lib/oneview-sdk/resource/api500/c7000/volume.rb

+        # Check if a resource exists
+        # @note one of the UNIQUE_IDENTIFIERS, e.g. name or uri or properties['name'], must be specified in the resource
+        # @return [Boolean] Whether or not resource exists
+        def exists?


This method does same thing that retrieve! method, right?
Maybe can be good call the retrieve! method here (Just do not use the self instance to call retrieve!, you can use a temp variable to that), avoiding write the "same" code.

In that case I think not, because exists? does not set the resource. Perhaps the opposite, the exists? being called within retrieve!.

Using exists? within retrieve is likely the better alternative indeed

I can see now. If I want to reuse only two first rows of each method. I need to call retrieve! inside exists?, because I need a result to set up the data. Can I use the Ricardo's suggestion, which uses a temporary variable to avoid setting the data when exists? is called. I don't see big gains from this change. But it's okay, I'll do this and see what the code will look like.

If you do not see big gains, keep the code as is it. No problem for me. =)
Important is the behavior to be ok.

I don't see big gains from this change. But it's okay, I'll do this and see what the code will look like.

Ops, true. Since exists? will only return a boolean instead of the resource, this is actually bad.

ricardogpsf · 2017-09-27T14:07:54Z

lib/oneview-sdk/resource/api500/c7000/volume.rb

+        # Gets the volume
+        # @raise [OneviewSDK::IncompleteResource] if the name parameter is not set
+        # @return [Array] the array of volumes
+        def find


The find name of this method seems generic, maybe marking that as a private or renaming the method can be better. Or, I guess be good option, you can put the code "to find by name" (It is what this method does) into the retrieve! method, it is a simples code and it is used just in this case.

This method is already a private method. But I can rename it.

Not sure what a good name for this could be though, find_by_name_in_properties ? Kinda long, but...

fgbulsoni · 2017-09-27T16:55:26Z

lib/oneview-sdk/resource/api500/c7000/volume.rb

+        # @return [Boolean] Whether or not resource exists
+        def exists?
+          return super unless @data['properties']
+          results = find


find.size == 1 ? true : false

or just:

find.size == 1

fgbulsoni · 2017-09-27T16:56:54Z

lib/oneview-sdk/resource/api500/c7000/volume.rb

+        def retrieve!
+          return super unless @data['properties']
+          results = find
+          if results.size == 1


return false unless results.size == 1 set_all(results.first.data) true

fgbulsoni · 2017-09-27T17:09:17Z

lib/oneview-sdk/resource/api500/c7000/volume.rb

+        # @raise [OneviewSDK::IncompleteResource] if the name parameter is not set
+        # @return [Array] the array of volumes
+        def find
+          @data['properties'] = Hash[@data['properties'].map { |k, v| [k.to_s, v] }]


Do we really want to be converting the whole @data['properties'] here?
This is likely less expensive and will accomplish the same thing:

name = @data['name'] || @data[:name]

aalexmonteiro · 2017-09-27T17:43:19Z

With your suggestions, the code is better now. Thanks guys. Can you review again, please?

fgbulsoni

fgbulsoni

While all the code is good and all, it is likely better to prepare to release this right away since Chef is depending on it, right?

fgbulsoni · 2017-09-28T13:15:14Z

CHANGELOG.md

@@ -1,3 +1,8 @@
+## v5.0.5(unreleased)


Drop the unreleased part

fgbulsoni

Looks good, feel free to release 👍

Overwriting the methods retrieve and exists in the volume.

0f90a17

aalexmonteiro added API500 enhancement labels Sep 26, 2017

aalexmonteiro requested review from fgbulsoni and ricardogpsf September 26, 2017 21:03

fgbulsoni reviewed Sep 27, 2017

View reviewed changes

Adjusting the issue title in CHANGELOG

0c3dde5

ricardogpsf reviewed Sep 27, 2017

View reviewed changes

fgbulsoni suggested changes Sep 27, 2017

View reviewed changes

Refactoring the retrieve! and exists? methods in the volume

c648428

ricardogpsf approved these changes Sep 27, 2017

View reviewed changes

fgbulsoni approved these changes Sep 28, 2017

View reviewed changes

fgbulsoni suggested changes Sep 28, 2017

View reviewed changes

CHANGELOG.md Outdated

@@ -1,3 +1,8 @@

## v5.0.5(unreleased)

Copy link

Contributor

fgbulsoni Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the unreleased part

Changing the SDK version

d2ae558

fgbulsoni approved these changes Sep 28, 2017

View reviewed changes

aalexmonteiro merged commit ceb3514 into master Sep 28, 2017

aalexmonteiro deleted the enhancement/find_volume_in_api500 branch September 28, 2017 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overwriting the methods retrieve! and exists? in the volume #274

Overwriting the methods retrieve! and exists? in the volume #274

aalexmonteiro commented Sep 26, 2017

fgbulsoni Sep 27, 2017

ricardogpsf Sep 27, 2017

aalexmonteiro Sep 27, 2017

fgbulsoni Sep 27, 2017

aalexmonteiro Sep 27, 2017

ricardogpsf Sep 27, 2017

fgbulsoni Sep 27, 2017 •

edited

Loading

ricardogpsf Sep 27, 2017

aalexmonteiro Sep 27, 2017 •

edited

Loading

fgbulsoni Sep 27, 2017 •

edited

Loading

fgbulsoni Sep 27, 2017 •

edited

Loading

ricardogpsf Sep 27, 2017

fgbulsoni Sep 27, 2017

fgbulsoni Sep 27, 2017

aalexmonteiro commented Sep 27, 2017

fgbulsoni left a comment

fgbulsoni left a comment

fgbulsoni Sep 28, 2017

fgbulsoni left a comment

Overwriting the methods retrieve! and exists? in the volume #274

Overwriting the methods retrieve! and exists? in the volume #274

Conversation

aalexmonteiro commented Sep 26, 2017

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgbulsoni Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexmonteiro Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

fgbulsoni Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

fgbulsoni Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexmonteiro commented Sep 27, 2017

fgbulsoni left a comment

Choose a reason for hiding this comment

fgbulsoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgbulsoni left a comment

Choose a reason for hiding this comment

fgbulsoni Sep 27, 2017 •

edited

Loading

aalexmonteiro Sep 27, 2017 •

edited

Loading

fgbulsoni Sep 27, 2017 •

edited

Loading

fgbulsoni Sep 27, 2017 •

edited

Loading