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

Raise an error if a region is not configured for central admin #12264

Conversation

carbonin
Copy link
Member

Now when someone tries to do something with an object that belongs to a remote region and hasn't configured the authentication key yet for that region, they will get a clear error message.

Previously the error was Error caught: [NoMethodError] undefined method '[]' for false:FalseClass which is not very helpful.

https://bugzilla.redhat.com/show_bug.cgi?id=1389536

@bdunne please review

Now when someone tries to do something with an object that belongs
to a remote region and hasn't configured the authentication key yet
for that region they will get a clear error message.

https://bugzilla.redhat.com/show_bug.cgi?id=1389536
@miq-bot
Copy link
Member

miq-bot commented Oct 27, 2016

Checked commits carbonin/manageiq@505dcd2~...d08652d with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 1 offense detected

spec/models/mixins/inter_region_api_method_relay_spec.rb

Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

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

👍 LGTM other than random comment about test setup...

let(:region_seq_start) { ApplicationRecord.rails_sequence_start }
let(:request_user) { "test_user" }
let(:api_connection) { double("ManageIQ::API::Client connection") }
let(:region_auth_token) { double("MiqRegion API auth token") }

before do
FactoryGirl.create(:miq_region, :region => ApplicationRecord.my_region_number)
expect(MiqRegion).to receive(:find_by).with(:region => region_number).and_return(region)
Copy link
Member

Choose a reason for hiding this comment

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

Do you even need this line? Doesn't 168 and 169 take care of that naturally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so too, but the object returned from FactoryGirl.create and the one returned by MiqRegion.find_by are different instances so I can't use the region defined by the let for expects without doing this.

If that's not what's going on, I agree, I would much rather not have this here 😄

Copy link
Member

Choose a reason for hiding this comment

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

Ahh when I remove this, specs fail because unless you have a region file it'll be region 0 (invalid), makes sense I suppose. ¯_(ツ)_/¯

let(:region_seq_start) { ApplicationRecord.rails_sequence_start }
let(:request_user) { "test_user" }
let(:api_connection) { double("ManageIQ::API::Client connection") }
let(:region_auth_token) { double("MiqRegion API auth token") }

before do
FactoryGirl.create(:miq_region, :region => ApplicationRecord.my_region_number)
expect(MiqRegion).to receive(:find_by).with(:region => region_number).and_return(region)
Copy link
Member

Choose a reason for hiding this comment

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

Ahh when I remove this, specs fail because unless you have a region file it'll be region 0 (invalid), makes sense I suppose. ¯_(ツ)_/¯

@chrisarcand chrisarcand merged commit 650c968 into ManageIQ:master Oct 28, 2016
@chrisarcand chrisarcand assigned chrisarcand and unassigned bdunne Oct 28, 2016
@chrisarcand chrisarcand added this to the Sprint 49 Ending Nov 14, 2016 milestone Oct 28, 2016
chessbyte pushed a commit that referenced this pull request Nov 1, 2016
…nfigured_regions

Raise an error if a region is not configured for central admin
(cherry picked from commit 650c968)

https://bugzilla.redhat.com/show_bug.cgi?id=1390405
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit 047b557007cadf9b4333ab8568562824d62ebeb0
Author: Chris Arcand <[email protected]>
Date:   Fri Oct 28 10:01:22 2016 -0400

    Merge pull request #12264 from carbonin/raise_for_api_calls_to_non_configured_regions

    Raise an error if a region is not configured for central admin
    (cherry picked from commit 650c968e99579d8ba3da04b8e3dc68645ac91368)

    https://bugzilla.redhat.com/show_bug.cgi?id=1390405

@carbonin carbonin deleted the raise_for_api_calls_to_non_configured_regions branch November 2, 2016 13:56
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.

5 participants