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

Refactoring available_os_types method for miq_ae_orchestrations. #12211

Merged
merged 2 commits into from
Nov 16, 2016
Merged

Refactoring available_os_types method for miq_ae_orchestrations. #12211

merged 2 commits into from
Nov 16, 2016

Conversation

pkomanek
Copy link
Contributor

Purpose or Intent

Refactoring the available_os_types method for the miq_ae_orchestration based on the issue bellow.

Links [Optional]

#12038

@pkomanek
Copy link
Contributor Author

@miq-bot add_labels automate, enhancement

@pkomanek
Copy link
Contributor Author

@miq-bot add_label refactoring

@@ -1,28 +1,46 @@
#
# Description: provide the dynamic list content from available operating systems
#
os_list = {'unknown' => '<Unknown>', 'linux' => 'Linux', 'windows' => 'Windows'}
selected_os = 'unknown'
class AvailableOsTypes
Copy link
Contributor

Choose a reason for hiding this comment

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

@Fryguy @pkomanek @gmcculloug
@Fryguy had a suggestion that the classes should be namespaced so that we don't have conflicts when testing. should we create the class name based on the path
e.g. for this one ManageIQ::Cloud::Orchestration::Operations::AvailableOsTypes or
ManageIQ::Cloud::Orchestration::Operations::Methods::AvailableOsTypes.

@Fryguy please suggest.
Is the preferred way to have

module ManageIQ
  module Cloud
       module Orchestration
           module Operations
                module Methods
                     class AvailableOsTypes
                     end
          ...
          end

or

module ManageIQ::Cloud::Orchestration::Operations::Methods
  class AvailableOsTypes
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

Gah I never sent the response @mkanoor :(


Although there are subtle differences, I would prefer the latter for readability.
Also, you should be careful in the ManageIQ namespace, because that is used by the ManageIQ::Api::Client and will probably also be used by the various provider gems at some point. You might want to stuff it into a ManageIQ::Automate top level namespace or something like that e.g:

module ManageIQ::Automate
  module Cloud::Orchestration::Operations::Methods
    class AvailableOsTypes
ennnd

Copy link
Contributor

Choose a reason for hiding this comment

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

@pkomanek
Can we follow Jason's suggestion and create everything with namespaces. The call at the bottom of the file and the specs would have to change.

@pkomanek pkomanek changed the title Refactoring available_os_types method for miq_ae_orchestrations. [WIP]Refactoring available_os_types method for miq_ae_orchestrations. Nov 7, 2016
@pkomanek
Copy link
Contributor Author

pkomanek commented Nov 7, 2016

@miq-bot add_label wip

@miq-bot miq-bot added the wip label Nov 7, 2016
@pkomanek
Copy link
Contributor Author

pkomanek commented Nov 7, 2016

@mkanoor Could be like this? The namespaces are not in inline syntax, because in this case they are not created and I get back this error: NameError: uninitialized constant ManageIQ::Automate::Cloud. I am open to any suggestion.

@pkomanek
Copy link
Contributor Author

pkomanek commented Nov 7, 2016

@mkanoor I could create ManageIQ::Automate::Cloud::Orchestration::Operations module somewhere(I dont know where) and after that I might use that inline syntax(same for other methods):

module Cloud::Orchestration::Operations::Methods
  class AvailableOsTypes
  end
end

@pkomanek pkomanek changed the title [WIP]Refactoring available_os_types method for miq_ae_orchestrations. Refactoring available_os_types method for miq_ae_orchestrations. Nov 9, 2016
@pkomanek
Copy link
Contributor Author

pkomanek commented Nov 9, 2016

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Nov 9, 2016
@miq-bot
Copy link
Member

miq-bot commented Nov 11, 2016

Checked commits pkomanek/manageiq@099bc0c~...f49398d with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@mkanoor mkanoor merged commit 0e2f788 into ManageIQ:master Nov 16, 2016
@pkomanek pkomanek deleted the refactoring_available_os_types branch November 16, 2016 17:05
@chessbyte chessbyte added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants