HewlettPackard / oneview-chef

This project is no longer being developed and has limited support. In the near future this repository will be fully deprecated. Please consider using other OneView projects, such as Terraform and Ansible Collection
https://supermarket.chef.io/cookbooks/oneview
Apache License 2.0
17 stars 15 forks source link

Support multiple API versions #97

Closed jsmartt closed 7 years ago

jsmartt commented 7 years ago

Scenario/Intent

Support more API versions and the new/updated resources associated with them

Environment Details

Description

In order to support newer API versions and yet maintain compatibility with previous ones, we'll need to do a pretty big refactor. The work is already underway, but this issue can capture some of the ideas and thoughts behind it, and serve as a discussion forum. From what we've discussed already, this refactor will likely include:

Outstanding Questions

  1. How do we handle resources that have new/updated properties for newer API versions? For example, for oneview_server_profile, how do we add an os_deployment_image property to the API300 provider but make it clear that it isn't used in the API200 provider? Seems like there's a lot of room for confusion here.
  2. Similarly, how should completely new resources be handled that don't exist in API200 (e.g., all the I3S stuff)?
  3. Is there a good way to support multiple versions and variants in a chef-client run? I haven't found a good solution for it yet, but it may be possible.
jsmartt commented 7 years ago

And as I was typing that... PR #96 was created lol

tmiotto commented 7 years ago

I have some quick solutions, that may not be the best, but they could work regarding the first two questions.

  1. Regarding the separate properties. Maybe we could do something similar to the ResourceBaseProperties::load in each provider. But I have some doubts if it would really work.

  2. We don't need to reference them in API200. I guess, we could just override the missing method to get all the constants and warn the user what API versions support this resource.

jsmartt commented 7 years ago

Yeah, I doubt we could load the properties at runtime, so I think it's probably OK to have them share a list that may not necessarily be exactly what the provider module expects. We could have a method to check to make sure no unused properties are specified though. So each provider module has an array constant of allowable properties, and if it's called with something else, it would print a warning or error.

For the second one, I agree also. We'd just have to have a check that makes it fail with a helpful message saying it's not supported in that API version.

jsmartt commented 7 years ago

You know, the more I think about this, the more I think it might be worth determining the API module at runtime. It will be a little more work making all the helper methods receive a context, but I think in the long run it will be best. It would basically come down to each resource file looking something like:

# resources/ethernet_network

OneviewCookbook::ResourceBaseProperties.load(self)
# Load any additional properties specific to this resource

action :create do
  # Determine correct API module based on properties passed in (or a default if none are given), 
  # and return the specified resource class for that module.
  klass = OneviewCookbook::Helper.get_resource_class(self, 'EthernetNetwork')

  # Make a new instance of that class so we can keep track of data in instance variables,
  # as well as validate that the given properties are valid.
  # Note that this step is probably not necessary, but should help clean things up.
  res = klass.new(self)

  # Now call the action method for that class
  res.create
end

...

That's the easy part, I know, but I think it'll make a clean interface for the resources. I haven't tried to implement this, so there may be dragons down that road, but we'll see next week. What do you think @tmiotto ?