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.

Sign up for GitHub

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

Jump to bottom

[WIP] Unifying the entity saving functions in save_inventory_container.rb. #6191

Closed
wants to merge 1 commit into from
Closed

[WIP] Unifying the entity saving functions in save_inventory_container.rb. #6191

wants to merge 1 commit into from

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jan 14, 2016

Try to unify save_selector_parts_inventory and
save_node_selector_parts_inventory in
app/models/ems_refresh/save_inventory_container.rb.

@simon3z
Copy link
Contributor

simon3z commented Jan 14, 2016

@dkorn @moolitayer please review. Thanks!

@miq-bot add_label providers/containers, refactoring

@simon3z
Copy link
Contributor

simon3z commented Jan 14, 2016

@dkorn @moolitayer it would have been great if we had tests for nodeSelector as well, sadly the vcr yaml doesn't have a pod with that.

Do we have a spec test elsewhere that may be relevant? Any suggestion for @yaacov to add testing for that?

Thanks.

@simon3z
Copy link
Contributor

simon3z commented Jan 14, 2016

@yaacov I think there should be a way to get unified_save_entity_inventory being called in all cases, so we can drop both save_selector_parts_inventory and save_node_selector_parts_inventory.

@yaacov
Copy link
Member Author

yaacov commented Jan 14, 2016

@simon3z the function save_child_inventory call "save_#{k}inventory" using the key, we do not have to write all possible "save#{k}_inventory" functions ?

@simon3z
Copy link
Contributor

simon3z commented Jan 14, 2016

@simon3z the function save_child_inventory call "save_#{k}inventory" using the key, we do not have to write all possible "save#{k}_inventory" functions ?

So can't you just change :node_selector_parts to :selector_parts and keep only save_selector_parts_inventory? (Removing save_node_selector_parts_inventory)

IIRC there's a catch there (cc @dkorn), it can't be that easy.

@chessbyte chessbyte added the wip label Jan 14, 2016
@yaacov
Copy link
Member Author

yaacov commented Jan 15, 2016

@simon3z suggested removing :node_selector_parts
this may code ambiguity between :selector_parts of container_service and container_group, is it ok ?

@yaacov
Copy link
Member Author

yaacov commented Jan 18, 2016

save_node_selector_parts_inventory is romved.
but the name node_selector and node_selector_parts is used to represent ems.container_groups selector_parts in the event and haml files :-(

simon3z
simon3z reviewed Jan 18, 2016
View reviewed changes
app/models/container_group.rb
has_many :selector_parts, -> { where(:section => "selectors") }, :class_name => CustomAttribute,
:as => :resource, :dependent => :destroy
has_many :container_conditions, :class_name => ContainerCondition,
:as => :container_entity, :dependent => :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov is this just different indentation? Please don't mix these changes because it's hard to understand what you're changing here.

@simon3z
Copy link
Contributor

simon3z commented Jan 18, 2016

@yaacov please squash all the patches... the PR should have just 1 patch.

@yaacov yaacov force-pushed the master branch 3 times, most recently from eff7663 to 3dfccc7 Compare January 19, 2016 06:21
@yaacov
Copy link
Member Author

yaacov commented Jan 19, 2016

  1. app/models/container_group.rb: change node_selector_parts to selector_parts. Rubocop had issues with the long line.
  2. squashed :-)

@simon3z I do not know the "real-world" meaning of node_selector vs. selector. The rspec passes, but do this changes have ok "real-world" meaning ?

@moolitayer
Copy link

Please note I'm working on a big overhaul in save_inventory_container in preparation for the disconnect feature.
This is a good effort regardless.

I will post a link once it is ready.

@yaacov
Copy link
Member Author

yaacov commented Jan 19, 2016

@moolitayer Thanks for the heads up :-)

@miq-bot
Copy link
Member

miq-bot commented Jan 19, 2016

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

@yaacov
Copy link
Member Author

yaacov commented Jan 20, 2016

@simon3z this PR is outdated by @moolitayer s work, do we need it ?

@carbonin
Copy link
Member

@yaacov @simon3z Just trying to debug an issue with the bot that seems to have come up with this PR. What caused the bot to add the gem changes label? Also the bot saw this PR referencing this BZ. Did something happen during the rebase that could have caused that?

Thanks

simon3z
simon3z reviewed Jan 20, 2016
View reviewed changes
app/models/container_group.rb
@@ -11,7 +11,7 @@ class ContainerGroup < ActiveRecord::Base
has_many :container_definitions, :dependent => :destroy
belongs_to :ext_management_system, :foreign_key => "ems_id"
has_many :labels, -> { where(:section => "labels") }, :class_name => CustomAttribute, :as => :resource, :dependent => :destroy
has_many :node_selector_parts, -> { where(:section => "node_selectors") }, :class_name => "CustomAttribute", :as => :resource, :dependent => :destroy
has_many :selector_parts, -> { where(:section => "selectors") }, :class_name => CustomAttribute, :as => :resource, :dependent => :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov @moolitayer @dkorn don't we need a migration script here? What about the current installations?

Copy link
Member Author

Choose a reason for hiding this comment

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

@simon3z Current installations having containerGroups with selector_parts section => "node_selectors" will have to migrate to section => "selectors", does this make sense ? what is a node_selector and what is a selector ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@simon3z Added a migration file, need a review because I do not know how to test this :-(

@yaacov
Copy link
Member Author

yaacov commented Jan 20, 2016

@carbonin "Did something happen during the rebase that could have caused that?"
During the rebase gems where added, had to run bundle install before running tests.

Sorry for the mess, @simon3z I think this PR is outdated, do we need to close it ?

@yaacov yaacov force-pushed the master branch 5 times, most recently from 4a38542 to c878596 Compare January 20, 2016 17:47
@yaacov yaacov force-pushed the master branch 2 times, most recently from 791b504 to 4d4bd03 Compare January 21, 2016 09:26
@yaacov
Unifying the save_node_selector_parts_inventory and save_selector_par…
40085c6 
…ts_inventory.

By changing node_selector... symbols to selector... symbols.
@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2016

Checked commit yaacov@40085c6 with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
5 files checked, 1 offense detected

app/models/container_group.rb

@yaacov
Copy link
Member Author

yaacov commented Feb 7, 2016

Closing PR because

  1. I made it on my master branch, and now want to clean it :-)
  2. @dkorn say node_selectors have different meaning then selectors
  3. It is made redundant by @moolitayer work

@yaacov yaacov closed this Feb 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dependencies providers/containers refactoring wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yaacov @simon3z @moolitayer @miq-bot @carbonin @chessbyte