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

Prevent queueing things for a zone that doesn't exist in the region #17987

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Sep 13, 2018

@bdunne bdunne force-pushed the ensure_zone_is_valid branch 4 times, most recently from 0d1d1b0 to c388f33 Compare September 13, 2018 22:03
it "without a matching region" do
expect { MiqQueue.create!(:state => "ready", :zone => "Missing Zone") }.to raise_error(ActiveRecord::RecordInvalid)
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.

@bdunne I glanced at the tests but didn't see one for a message with a nil zone... does that pass this validation and not raise an error?

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

@miq-bot
Copy link
Member

miq-bot commented Sep 14, 2018

Checked commit bdunne@bf66236 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🍰

@jrafanie
Copy link
Member

LGTM for master. cc @gtanzillo

@jrafanie jrafanie added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 14, 2018
@jrafanie jrafanie self-assigned this Sep 14, 2018
@jrafanie jrafanie merged commit 01c28d5 into ManageIQ:master Sep 14, 2018
@bdunne bdunne deleted the ensure_zone_is_valid branch September 14, 2018 19:33
borod108 pushed a commit to borod108/manageiq-providers-ovirt that referenced this pull request Sep 16, 2018
Fixing a test that was broken due to a PR that added validation to zone
when putting a task into MiqQueue. (ManageIQ/manageiq#17987)
@ZitaNemeckova
Copy link
Contributor

Fix for ui-classic ManageIQ/manageiq-ui-classic#4669 . Is this gaprindashvili/yes?

@simaishi
Copy link
Contributor

simaishi commented Oct 8, 2018

@bdunne As per ManageIQ/manageiq-api#472 (comment), I'm assuming this should be gaprindashvili/yes. But cherry-pick conflicts on 4 spec files:

Changes to be committed:

	modified:   app/models/miq_queue.rb
	modified:   spec/models/miq_action_spec.rb
	modified:   spec/models/miq_queue_spec.rb
	modified:   spec/models/miq_request_spec.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   spec/lib/extensions/ar_nested_count_by_spec.rb
	both modified:   spec/lib/tasks/evm_application_spec.rb
	both modified:   spec/models/miq_task_spec.rb
	both modified:   spec/models/mixins/authentication_mixin_spec.rb

Can you please create a separate PR for Gaprindashvili branch?

bdunne pushed a commit to bdunne/manageiq that referenced this pull request Oct 8, 2018
Prevent queueing things for a zone that doesn't exist in the region
https://bugzilla.redhat.com/show_bug.cgi?id=1635764

(cherry picked from commit 01c28d5)
@JPrause
Copy link
Member

JPrause commented Oct 10, 2018

@bdunne is this still in conflict for gaprindashvili backport?

@simaishi
Copy link
Contributor

Backported to Gaprindashvili via #18068

cben pushed a commit to cben/manageiq-providers-kubernetes that referenced this pull request Nov 1, 2018
…_failing_on_missing_zone

Fix MetricsCapture and Refresher specs failing due to a missing zone

Stubbing MiqServer.my_zone to return a dummy string was affecting the zone
with which events were added to MiqQueue, which broke now that MiqQueue
requires a real zone (ManageIQ/manageiq#17987).
cben pushed a commit to cben/manageiq-providers-openshift that referenced this pull request Nov 1, 2018
…ing_zone

Fix specs failing on a missing zone

Stubbing MiqServer.my_zone to return a dummy string was affecting the zone
with which events were added to MiqQueue, which broke now that MiqQueue
requires a real zone (ManageIQ/manageiq#17987).
cben added a commit to cben/manageiq-providers-kubernetes that referenced this pull request Nov 1, 2018
Stubbing MiqServer.my_zone to return a dummy string was affecting the zone
with which events were added to MiqQueue, which broke now that MiqQueue
requires a real zone (ManageIQ/manageiq#17987).
@kbrock
Copy link
Member

kbrock commented Nov 13, 2019

Looks like this is generating a sql call for every insert into miq_queue.
We're taking a pretty big hit here.

Wonder if we want to do some sort of caching for this value

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.

7 participants