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

Adding Datawarehouse management #12205

Merged
merged 2 commits into from
Nov 29, 2016
Merged

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented Oct 26, 2016

Adding Datawarehouse management.
Commits:

  • Datawarehouse manager with an empty refresh
  • Ems Datawarehouse add/delete/display

Based on bits from the current hawkular manager.

Intent: A new provider in the system. The end goal is using it to attach events, metrics and inventory to other providers in the system. One Datawarehouse manager can serve many target providers. Currently there is one hawkular implementation. elastic search is planned next.

To view the new provider screens in the UI, set in settings.yml (or settings.local.yml)

:product:
  :datawarehouse_manager: true
add

dwh_add

list

dwh_list

view

3

@Fryguy
Copy link
Member

Fryguy commented Oct 26, 2016

@blomquisg @Ladas @durandom Please also review.

@moolitayer
Copy link
Author

cc @simon3z @cben

class DatawarehouseManager < BaseManager
class << model_name
def route_key
"ems_datawarehouse"
Copy link
Member

Choose a reason for hiding this comment

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

@martinpovolny is this a problem, that route key and singular_route_key are the same?

I see that MiddlewareManager has that too.

Copy link
Author

Choose a reason for hiding this comment

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

@martinpovolny @himdel It seems to only works this way, I'm not what are the implications...

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly me neither, but both CloudManager and MiddlewareManager do a similar thing, so.. :)

Copy link
Member

Choose a reason for hiding this comment

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

so... leave until it breaks :)
no, ok, fine then

@Ladas
Copy link
Contributor

Ladas commented Oct 31, 2016

the definitions look correct 👍

@moolitayer you have several valid CI failures there, let us know if you need any help with fixing them :-)

@moolitayer
Copy link
Author

Thanks @Ladas. I started fixing those. I'm not sure why I'm getting this:

  1) ExtManagementSystem validates across sub-classes, from vmware to manageiq/providers/hawkular/datawarehouse_manager duplicate hostname
     Failure/Error: manager = FactoryGirl.build(t, :hostname => @ems_vmware.hostname)

     ArgumentError:
       Factory not registered: manageiq/providers/hawkular/datawarehouse_manager
     # /home/mtayer/.rvm/gems/ruby-2.3.0/gems/factory_girl-4.5.0/lib/factory_girl/registry.rb:24:in `find'
     # /home/mtayer/.rvm/gems/ruby-2.3.0/gems/factory_girl-4.5.0/lib/factory_girl/decorator.rb:10:in `method_missing'
     # /home/mtayer/.rvm/gems/ruby-2.3.0/gems/factory_girl-4.5.0/lib/factory_girl.rb:76:in `factory_by_name'
     # /home/mtayer/.rvm/gems/ruby-2.3.0/gems/factory_girl-4.5.0/lib/factory_girl/factory_runner.rb:12:in `run'
     # /home/mtayer/.rvm/gems/ruby-2.3.0/gems/factory_girl-4.5.0/lib/factory_girl/strategy_syntax_method_registrar.rb:20:in `block in define_singular_strategy_method'
     # ./spec/models/ext_management_system_spec.rb:414:in `block (6 levels) in <top (required)>'

@moolitayer
Copy link
Author

cc @lucasponce @pilhuhn

@moolitayer moolitayer changed the title Datawarehouse manager with an empty refresh Adding Datawarehouse management Nov 1, 2016
@Ladas
Copy link
Contributor

Ladas commented Nov 1, 2016

@moolitayer here it goes through all subclasses of ExtManagementSystem https://github.com/Ladas/manageiq/blob/7fb40688b4401f772490ec18f1e011660d51cd0b/spec/models/ext_management_system_spec.rb#L395-L395

So it takes automatically also your new Ems class. You can add the factory and make it pass, or just list it in the exceptions few lines below (with todo to add the specs later). Try if it's enough to add the factory, but I think you will need few actions implemented to pass some of those specs.

@moolitayer moolitayer changed the title Adding Datawarehouse management [WIP] Adding Datawarehouse management Nov 1, 2016
@moolitayer
Copy link
Author

@miq-bot add_label wip
Moving to work on event collection for a poc, will catch up on this

@miq-bot miq-bot added the wip label Nov 1, 2016
@miq-bot
Copy link
Member

miq-bot commented Nov 1, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@moolitayer moolitayer force-pushed the datawarehouse branch 2 times, most recently from fbe10a0 to 23aa5bb Compare November 1, 2016 13:31
@lucasponce
Copy link
Contributor

Hi @moolitayer I have asked @pilhuhn to review more the architectural aspects and the relation between the datawarehouse and middleware provider.
From my side I am going to review more details at implementation level.

@@ -3864,7 +3864,7 @@
:feature_type: admin
:identifier: middleware_deployment_restart

# Middleware Datasource
# Middleware Datasource0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 0 ?

@moolitayer
Copy link
Author

Back from reserves duty and continuing with this effort

@moolitayer
Copy link
Author

@durandom @Fryguy @lucasponce @blomquisg I got comments from several people on the name (datawarehouse) not being adequate. One comment was that datawarehouses is a specific term for things that persist history for very long time. I would like to go for something more generic like datastore.

@durandom
Copy link
Member

Datastore could be misleading as Vmware also has that name in it's terminology. But I get the point of DWH being to specific

@moolitayer moolitayer force-pushed the datawarehouse branch 5 times, most recently from 40108ef to b271c6f Compare November 16, 2016 14:30
@moolitayer moolitayer force-pushed the datawarehouse branch 3 times, most recently from 20b6c5f to f9aaeb7 Compare November 24, 2016 14:52
@moolitayer
Copy link
Author

@blomquisg I'm hoping this can be merged soon given that it is skeletal and hidden.
note this no longer has an sql migration(I can't seem to remove it manually).

@moolitayer moolitayer closed this Nov 24, 2016
@moolitayer moolitayer reopened this Nov 24, 2016
@moolitayer moolitayer force-pushed the datawarehouse branch 3 times, most recently from 67c86a7 to bd5bdae Compare November 27, 2016 20:35
@moolitayer moolitayer closed this Nov 28, 2016
@moolitayer moolitayer reopened this Nov 28, 2016
@moolitayer
Copy link
Author

@blomquisg can we merge this skeletal pr or is there anything you want changed?
(the provider screens are hidden)

@dclarizio
Copy link

@martinpovolny @h-kataria please suggest some specs that could be added to cover the UI areas added here. Thx, Dan

@miq-bot
Copy link
Member

miq-bot commented Nov 29, 2016

Checked commits moolitayer/manageiq@717d498~...04e5ef6 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
32 files checked, 19 offenses detected

app/helpers/application_helper/toolbar/ems_datawarehouse_center.rb

  • ❗ - Line 31, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 33, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 47, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 49, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 62, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 64, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 75, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 79, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ems_datawarehouses_center.rb

  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 45, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 47, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 65, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
  • ❗ - Line 67, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 85, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar_chooser.rb

app/presenters/menu/default_menu.rb

  • ❗ - Line 117, Col 13 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 119, Col 13 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

spec/controllers/ems_datawarehouse_controller_spec.rb

@moolitayer
Copy link
Author

@dclarizio I've added

  • ems_datawarehouse_controller_spec.rb
  • ems_datawarehouse_routing_spec.rb

There are changes to ext_management_system_spec.rb as well

Does this cover specs?

@moolitayer moolitayer closed this Nov 29, 2016
@moolitayer moolitayer reopened this Nov 29, 2016
@blomquisg
Copy link
Member

@Loicavenel @moolitayer @Fryguy @durandom

I'm fine with us starting this way (having a Data Warehouse Manager). But, I'm still not convinced this is the ultimate end goal. I'm thinking we will eventually likely re-brand this thing as a way to share data collection resources across managers. I'm still seeing a distinction between the things that we manage (Storage, Network, Containers, Compute) and the types of data we collect from those managed systems (inventory, events, and metrics). I see the Data Warehouse manager falling into the second group.

But, I don't really know what that looks like yet and I have no ideas how to even describe it in a meaningful way.

So, I'll merge this, we can move forward with this concept until we have a better notion of separating out these two ideas (if we ever figure it out 😄).

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.