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

Only reload settings for servers in current region #17992

Conversation

agrare
Copy link
Member

@agrare agrare commented Sep 18, 2018

When changing settings for a different region, only queue a settings
reload for servers in the current region.

Fixes test failures e.g https://travis-ci.org/ManageIQ/manageiq-api/jobs/430035513#L2033-L2045

@@ -62,7 +62,7 @@ def miq_servers
end

def servers_for_settings_reload
miq_servers.where(:status => "started")
MiqServer.in_my_region.where(:status => "started")
Copy link
Member

Choose a reason for hiding this comment

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

That's not accurate either.

For example, if we have current region 1 has server 1, and region 2 has server 2, and if someone makes a change to the settings for region 2, then this method will return server 1, which is wrong. It should instead return []. I think you can do miq_servers.in_my_region, which, while a strange mix, is accurate (probably would warrant a code comment). While it might be better to be explicit, and return a literal [], I'd prefer to keep the method chainable.

Copy link
Member Author

@agrare agrare Sep 18, 2018

Choose a reason for hiding this comment

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

miq_servers.in_my_region

Hm this is what I had first but thought it was weird to be chaining .in_region(region_number).in_my_region but yeah that is wrong

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

app/models/miq_region.rb Outdated Show resolved Hide resolved
@@ -62,7 +62,7 @@ def miq_servers
end

def servers_for_settings_reload
miq_servers.where(:status => "started")
MiqServer.in_my_region.where(:status => "started")
Copy link
Member

Choose a reason for hiding this comment

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

Won't this unnecessarily queue a settings reload for the servers in_my_region if some other region is updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bdunne already fixed per @Fryguy's comment #17992 (comment)

When changing settings for a different region, only queue a settings
reload for servers in the current region.

Fixes test failures e.g https://travis-ci.org/ManageIQ/manageiq-api/jobs/430035513#L2033-L2045
@agrare agrare force-pushed the fix_settings_reload_for_server_in_different_region branch from 7f81d66 to 3d43d72 Compare September 18, 2018 14:19
@miq-bot
Copy link
Member

miq-bot commented Sep 18, 2018

Checked commit agrare@3d43d72 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit 4a60b03 into ManageIQ:master Sep 18, 2018
@Fryguy Fryguy added the bug label Sep 18, 2018
@Fryguy Fryguy added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 18, 2018
@agrare agrare deleted the fix_settings_reload_for_server_in_different_region branch September 18, 2018 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants