Fixes #93: Logical Switch refresh conflict #136

ricardogpsf · 2017-01-23T13:46:38Z

Description

Changed the name of method to refresh_state and the conflict was fixed.

Issues Resolved

Fixes #93

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.

tmiotto

It would also need to change it in the /examples

tmiotto · 2017-01-23T13:48:41Z

lib/oneview-sdk/resource/api200/logical_switch.rb

@@ -49,7 +49,7 @@ def create
      # Updates this object using the data that exists on OneView
      # @note Will overwrite any data that differs from OneView
      # @return [Resource] self
-      def refresh
+      def refresh_data_state!


I would prefer calling it refresh_state to follow the pattern with the other refreshes

ricardogpsf · 2017-01-23T14:31:31Z

@tmiotto The method is not referenced in the examples. But I'll put it in there.

jsmartt · 2017-01-23T23:27:15Z

lib/oneview-sdk/resource/api200/logical_switch.rb

@@ -49,7 +49,7 @@ def create
      # Updates this object using the data that exists on OneView
      # @note Will overwrite any data that differs from OneView


Are these comments true? Looks like a copy-paste that we forgot to change.

Really, this comment is not good. I will use the documentation description.

jsmartt · 2017-01-23T23:30:22Z

examples/api200/logical_switch.rb

-logical_switch.set_switch_credentials('172.16.11.11', ssh_credentials, snmp_v1)
-logical_switch.set_switch_credentials('172.16.11.12', ssh_credentials, snmp_v1_2)
+logical_switch.set_switch_credentials('172.18.16.91', ssh_credentials, snmp_v1)
+logical_switch.set_switch_credentials('172.18.16.92', ssh_credentials, snmp_v1_2)


Should these IPs and credentials be moved into the _client.rb file?

yep. It is the better approach. :)
I'm working in this now.

- Method documentation comment was changed - The method returning the LogicalSwitch object

tmiotto · 2017-01-24T13:09:53Z

examples/api200/logical_switch.rb


+logical_switch_group = OneviewSDK::LogicalSwitchGroup.get_all(@client).first

 logical_switch = OneviewSDK::LogicalSwitch.new(
  @client,
  name: 'Teste_SDK',


I know it was not added right now, but could we change the name of this LogicalSwitch?

tmiotto · 2017-01-24T13:11:20Z

lib/oneview-sdk/resource/api200/logical_switch.rb

@@ -50,7 +50,7 @@ def create
      # @return [OneviewSDK::LogicalSwitch] self
      def refresh_state!
        response = @client.rest_put(@data['uri'] + '/refresh')
-        @client.response_handler(response)
+        body = @client.response_handler(response)
        set_all(body)
        self


Redundant self, set_all already returns self

oh, yeah! Changing that. =)

tmiotto · 2017-01-24T13:21:55Z

lib/oneview-sdk/resource/api200/logical_switch.rb

-      def refresh
+      # Reclaims the top-of-rack switches in a logical switch.
+      # @return [OneviewSDK::LogicalSwitch] self
+      def refresh_state!


Regarding the method implementation, I took a look in the API and I don't think the refresh/endpoint does what this method think it does.

I mean, the response does not returns the Logical Switch, it reclaims the associated ToR switches... So, it won't make sense using the set_all at all.

Also, since it does not changes anything in the resource, you will probably have to remove the ! from the method signature

But, the return of the request is a LogicalSwitch with associated data (Switches uris and SwitchGroup uris, etc).
And the http verb of request used in this method is the PUT (like documentation describes), I imagine that server can change the data of LogicalSwitch when applyes the refresh.
Please, tell me if I am with wrong undestanding.
What are you think?

Actually, the return is a task indicating wether the refresh is complete or not. If you want to return the Refreshed LogicalSwitch, you need to wait until the refresh state changes from Refreshing, then perform a retrieve!/refresh so this way you will have the refreshed LogicalSwitch.

The changes was done.

…itch)

ricardogpsf · 2017-01-25T17:32:58Z

The changes was done.

tmiotto · 2017-01-27T12:21:56Z

lib/oneview-sdk/resource/api200/logical_switch.rb

@@ -47,11 +47,10 @@ def create
      end

      # Reclaims the top-of-rack switches in a logical switch.
-      # @return [OneviewSDK::LogicalSwitch] self
-      def refresh_state!
+      # @return [Hash] Http response body with logical switch data.


The REST call returns only a task, not the logical switch. I guess you have two alternatives:

Remove the comment with the @return since it would be useless.

After the REST call use the refresh or retrieve! method to update the resource

But, the @client.response_handler(response) is called that waits for task finish and return the LogicalSwitch Hash.

Anyway, I did the first option. =)

tmiotto

Good to go now 👍

Fixes #93: Logical Switch refresh conflict

be6d009

tmiotto suggested changes Jan 23, 2017

View reviewed changes

Changed name of a method in Logical Switch

9c927d3

jsmartt mentioned this pull request Jan 23, 2017

CHANGELOG notes for v3.1.0 changed after the release #138

Closed

ricardogpsf added 4 commits January 23, 2017 13:23

Merge branch 'master' into bugfix/logical_switch_resfresh

afdf699

Added examples for refresh_state! of Logical Switch

5401f01

Fixed Style/StringLiterals problems in examples of Logical Switch

5abfc6a

Refactor text in Changelog about bugfix of LogicalSwitch

a5545bb

jsmartt reviewed Jan 23, 2017

View reviewed changes

jsmartt added breaking_change bug/bug-fix labels Jan 23, 2017

ricardogpsf added 2 commits January 24, 2017 09:18

Changed method refresh_state! of LogicalSwitch

915a3e9

- Method documentation comment was changed - The method returning the LogicalSwitch object

Exported literal values used in example to variables (for LogicalSwitch)

972655f

tmiotto suggested changes Jan 24, 2017

View reviewed changes

applying changes to LogicalSwitch based on PR comments

c8a9338

tmiotto suggested changes Jan 24, 2017

View reviewed changes

Refactored the name of method refresh_state and its return (LogicalSw…

4e99590

…itch)

tmiotto suggested changes Jan 27, 2017

View reviewed changes

Removed doc comment about return of method refresh_state (LogicalSwitch)

a698113

tmiotto approved these changes Jan 27, 2017

View reviewed changes

ricardogpsf merged commit ceccdb5 into master Jan 30, 2017

ricardogpsf deleted the bugfix/logical_switch_resfresh branch January 30, 2017 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #93: Logical Switch refresh conflict #136

Fixes #93: Logical Switch refresh conflict #136

ricardogpsf commented Jan 23, 2017 •

edited by jsmartt

Loading

tmiotto left a comment

tmiotto Jan 23, 2017

ricardogpsf commented Jan 23, 2017 •

edited

Loading

jsmartt Jan 23, 2017

ricardogpsf Jan 24, 2017

jsmartt Jan 23, 2017

ricardogpsf Jan 24, 2017 •

edited

Loading

tmiotto Jan 24, 2017

tmiotto Jan 24, 2017

ricardogpsf Jan 24, 2017

tmiotto Jan 24, 2017

ricardogpsf Jan 24, 2017 •

edited

Loading

tmiotto Jan 24, 2017

ricardogpsf Jan 25, 2017

ricardogpsf commented Jan 25, 2017

tmiotto Jan 27, 2017

ricardogpsf Jan 27, 2017 •

edited

Loading

ricardogpsf Jan 27, 2017

tmiotto left a comment

		@@ -49,7 +49,7 @@ def create
		# Updates this object using the data that exists on OneView
		# @note Will overwrite any data that differs from OneView

Fixes #93: Logical Switch refresh conflict #136

Fixes #93: Logical Switch refresh conflict #136

Conversation

ricardogpsf commented Jan 23, 2017 • edited by jsmartt Loading

Description

Issues Resolved

Check List

tmiotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardogpsf commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardogpsf Jan 24, 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

Choose a reason for hiding this comment

ricardogpsf Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardogpsf commented Jan 25, 2017

Choose a reason for hiding this comment

ricardogpsf Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmiotto left a comment

Choose a reason for hiding this comment

ricardogpsf commented Jan 23, 2017 •

edited by jsmartt

Loading

ricardogpsf commented Jan 23, 2017 •

edited

Loading

ricardogpsf Jan 24, 2017 •

edited

Loading

ricardogpsf Jan 24, 2017 •

edited

Loading

ricardogpsf Jan 27, 2017 •

edited

Loading