Synergy support option 2 #99

jsmartt · 2017-01-10T00:36:45Z

Description

This PR offers another option for addressing support for Synergy and future OV releases. (At this point I just have the ethernet_network providers done.) The logic for resources is split up into separate providers for each API version and variant. Here are the basic features:

Run-time loading of the correct SDK module, variant, and class.
Can use multiple SDK variants & API versions in a single Chef run
Can override the X-API-Version header in API requests by setting a property on a resource
Basically all the logic is split out into Provider classes, not modules
Provider classes expose many of the helper methods have been copied into the provider class so they can be easily used where they will be needed

Compared to #96, this option will require more work to refactor, but I think it's worth it.

Issues Resolved

Fixes #97

Check List

New functionality includes testing.
- All tests pass ($ rake test).
- New resources and/or actions have are included in the matchers.rb and matchers_spec.
New functionality has been documented in the README if applicable.
- New functionality has been thoroughly documented in examples (please include helpful comments).
Changes documented in the CHANGELOG.

@tmiotto or others, let me know what you think!

tmiotto · 2017-01-10T15:49:12Z

libraries/resources/api200/resource_provider.rb

+module OneviewCookbook
+  module API200
+    # Base class for API200 resource providers
+    class EthernetNetwork < OneviewCookbook::ResourceProvider


EthernetNetwork? I don't get it

Oops. Should be fixed now.

tmiotto · 2017-01-10T16:00:18Z

libraries/resources/api200/resource_provider.rb

+module OneviewCookbook
+  module API200
+    # Base class for API200 resource providers
+    class ResourceProvider < OneviewCookbook::ResourceProvider


Ok, but why do we have this?

It is to add any shared behavior specific to API200? If so, shouldn't we have to do something similar to API300 or C7000 and Synergy?

Yes. I didn't include the one for API300 and the variants YET, but yes, the concept is the same. We'd add the default class in there. Exactly like the Resource class for the SDK

tmiotto · 2017-01-10T16:16:03Z

libraries/.foodcritic

@@ -0,0 +1,4 @@
+~FC011


I guess foodcritic is failing because we're running it from the wrong directory.

Actually, it should receive as parameter the cookbooks/ folder, so it gets our oneview cookbook and then analyzes it. But in this case it is being executed inside oneview/, so this way it is interpreting the libraries as a separate cookbook, and not a subfolder

Well, it's not exactly the wrong directory, but running on cookbooks/ would solve the problem (but this is a workaround), or maybe we should just add an exclude_path if we don't want libraries to be analyzed by it.

I can't get it to not see the libraries directory as a separate cookbook, which is why I put that .foodcritic file in there. How are you able to run it so it gets ignored?

The travis test doesn't have a cookbooks directory. Just this repo.

I think the real issue is that foodcritic thinks the libraries directory is a cookbook because of the resources directory within it. We can rename the directory to avoid these issues. See this issue

Should be fixed now

tmiotto · 2017-01-10T16:39:56Z

These changes from the PR #96 should be added to stop the spec from failing.

jsmartt · 2017-01-11T00:22:30Z

Added some more tests and functionality. I think the functionality for EthernetNetworkProvider is now done. Just need a few more test, but I'll get to those tomorrow.

tmiotto · 2017-01-11T15:28:34Z

libraries/resource_providers/api300.rb

@@ -20,7 +20,7 @@ module API300
    # @return [Class] Resource class or nil if not found
    def self.resource_named(type, variant)
      raise "API300 variant #{variant} is not supported!" unless SUPPORTED_VARIANTS.include?(variant.to_s)
-      new_type = type.to_s.downcase.gsub(/[ -_]/, '')
+      new_type = type.to_s.downcase.gsub(/[ -_]/, '') + 'provider'


Should we still be calling it resource_named?

What do you suggest instead? resource_provider_named ?

It is an option, or maybe just provider_named.

tmiotto · 2017-01-11T15:30:34Z

spec/unit/resources/ethernet_network/create_spec.rb

@@ -27,3 +27,14 @@
    expect(real_chef_run).to create_oneview_ethernet_network('EthernetNetwork1')
  end
 end
+


Maybe we would not want to implement tests of things that are already being tested on API200

Agreed. This is just a sample of what you would do if you wanted to test API300 resources. I think this particular one is probably OK to leave in since it's the only one so far, but we wouldn't have to add these sort of tests for other resources that are the same as API200.

tmiotto · 2017-01-11T18:01:01Z

@jsmartt Just one more thought...

As far as we go through the implementation, we'll always be using the last API version as base for the next one, i.e. API300::X < API200::X, API500::X < API300::X.

Said that we won't be using any ResourceProvider other than the OneviewCookbook::ResourceProvider since API500::X < API300::X < API200::X < OneviewCookbook::ResourceProvider, and the ones created aren't adding anything new.

It won't be better to just have the OneviewCookbook::ResourceProvider?

jsmartt · 2017-01-11T18:04:00Z

Yeah, I guess I added the API200 sub-class prematurely. We don't need it now, so there's no need to include it. I'll remove it

jsmartt · 2017-01-11T20:13:26Z

OK @tmiotto , I'm ready on this when you are 😄

jsmartt added 6 commits December 13, 2016 19:36

Added support for API versions

78d90fd

Updated README

5f75402

Fixed line endings

318fee2

Split out api versions into providers

5d1d4f8

Updated attributes and docs

32b44bf

Fixed tests

751cd59

tmiotto reviewed Jan 10, 2017

View reviewed changes

jsmartt added 2 commits January 10, 2017 15:51

Fixed rubocop issues

1c739cb

Fixed API200::ResourceProvider

e9ae484

tmiotto reviewed Jan 10, 2017

View reviewed changes

Fixed foodcritic issues

6702135

tmiotto reviewed Jan 10, 2017

View reviewed changes

Renamed libraries/resources as libraries/resource_providers

e374d52

jsmartt added 2 commits January 10, 2017 16:55

Fix spec tests

583a730

Restored original functionality for other resources and added tests

72cf8e3

tmiotto reviewed Jan 11, 2017

View reviewed changes

jsmartt added 2 commits January 11, 2017 19:33

Finished EthNet provider

2936df0

Updated docs

d5c8f11

jsmartt assigned tmiotto Jan 11, 2017

tmiotto approved these changes Jan 12, 2017

View reviewed changes

tmiotto merged commit 7e0f5bd into master Jan 12, 2017

tmiotto mentioned this pull request Jan 12, 2017

Changes for Synergy support #96

Closed

jsmartt deleted the synergy_jared branch January 12, 2017 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synergy support option 2 #99

Synergy support option 2 #99

jsmartt commented Jan 10, 2017 •

edited

Loading

tmiotto Jan 10, 2017

jsmartt Jan 10, 2017

tmiotto Jan 10, 2017

jsmartt Jan 10, 2017 •

edited

Loading

tmiotto Jan 10, 2017

tmiotto Jan 10, 2017 •

edited

Loading

jsmartt Jan 10, 2017

jsmartt Jan 10, 2017

jsmartt Jan 10, 2017

jsmartt Jan 10, 2017

tmiotto commented Jan 10, 2017

jsmartt commented Jan 11, 2017

tmiotto Jan 11, 2017

jsmartt Jan 11, 2017

tmiotto Jan 11, 2017

tmiotto Jan 11, 2017

jsmartt Jan 11, 2017

tmiotto commented Jan 11, 2017

jsmartt commented Jan 11, 2017

jsmartt commented Jan 11, 2017

Synergy support option 2 #99

Synergy support option 2 #99

Conversation

jsmartt commented Jan 10, 2017 • edited Loading

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

jsmartt Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmiotto Jan 10, 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

tmiotto commented Jan 10, 2017

jsmartt commented Jan 11, 2017

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

tmiotto commented Jan 11, 2017

jsmartt commented Jan 11, 2017

jsmartt commented Jan 11, 2017

jsmartt commented Jan 10, 2017 •

edited

Loading

jsmartt Jan 10, 2017 •

edited

Loading

tmiotto Jan 10, 2017 •

edited

Loading