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

Move Graph Refresh Settings to EMS #17931

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Aug 30, 2018

Move inventory_object_refresh? and allow_targeted_refresh? to
ext_management_system so that code outside the Refresher can get away
from checking settings directly.

Follow-up to #17916

@agrare
Copy link
Member Author

agrare commented Aug 30, 2018

@gtanzillo found a few more places outside of the Refresher that were checking the settings directly.

@@ -78,6 +78,8 @@ def parse_targeted_inventory(ems, _target, inventory)
# ManagerRefresh::TargetCollection. This way we can do targeted refresh of all queued targets in 1 refresh
def preprocess_targets
@targets_by_ems_id.each do |ems_id, targets|
ems = ExtManagementSystem.find(ems_id)

if targets.any? { |t| t.kind_of?(ExtManagementSystem) }
ems = @ems_by_ems_id[ems_id]
Copy link
Member

Choose a reason for hiding this comment

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

Could this line just be moved to where you added the new line setting ems

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah definitely

@agrare agrare force-pushed the move_graph_refresh_settings_to_ems branch from b54e42b to ad3597a Compare August 30, 2018 21:00
@@ -718,6 +718,14 @@ def self.display_name(number = 1)
n_('Manager', 'Managers', number)
end

def inventory_object_refresh?
Settings.ems_refresh[emstype].try(:[], :inventory_object_refresh)
Copy link
Member

Choose a reason for hiding this comment

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

You could use Settings.ems_refresh.fetch_path(ems_type, :inventory_obeject_refresh) to avoid the try

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done

Move inventory_object_refresh? and allow_targeted_refresh? to
ext_management_system so that code outside the Refresher can get away
from checking settings directly.
@agrare agrare force-pushed the move_graph_refresh_settings_to_ems branch from ad3597a to 0aca388 Compare August 31, 2018 12:53
@miq-bot
Copy link
Member

miq-bot commented Aug 31, 2018

Checked commit agrare@0aca388 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍪

@bdunne bdunne merged commit 2f9a1dd into ManageIQ:master Aug 31, 2018
@bdunne bdunne added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 31, 2018
@bdunne bdunne assigned bdunne and unassigned gtanzillo Aug 31, 2018
@agrare agrare deleted the move_graph_refresh_settings_to_ems branch August 31, 2018 13:26
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.

4 participants