-
Notifications
You must be signed in to change notification settings - Fork 356
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
Fixed code that displays values saved for NTP Servers for selected zone #3568
Fixed code that displays values saved for NTP Servers for selected zone #3568
Conversation
dff18d4
to
d7bdfcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall 👍 , just some test comments.
it 'sets ntp server info for display' do | ||
controller.instance_variable_set(:@sb, :active_tab => 'settings_zone') | ||
controller.instance_variable_set(:@edit, :new => {:ntp => {:server => ["zone1.pool.com", "zone2.pool.com"]}}) | ||
@zone = FactoryGirl.create(:zone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an @
variable? It won't be reused outside of the test context, right?
controller.instance_variable_set(:@edit, :new => {:ntp => {:server => ["zone1.pool.com", "zone2.pool.com"]}}) | ||
@zone = FactoryGirl.create(:zone) | ||
controller.send(:zone_save_ntp_server_settings, @zone) | ||
@miq_server = FactoryGirl.create(:miq_server, :zone => @zone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@zone = FactoryGirl.create(:zone) | ||
controller.send(:zone_save_ntp_server_settings, @zone) | ||
@miq_server = FactoryGirl.create(:miq_server, :zone => @zone) | ||
allow(MiqServer).to receive(:my_server).and_return(@miq_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a helper to creates everything you need here. _guid, miq_server, zone = EvmSpecHelper.local_guid_miq_server_zone
context 'zone node' do | ||
it 'sets ntp server info for display' do | ||
controller.instance_variable_set(:@sb, :active_tab => 'settings_zone') | ||
controller.instance_variable_set(:@edit, :new => {:ntp => {:server => ["zone1.pool.com", "zone2.pool.com"]}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the example.org
or example.com
IANA example domains for test data.
d7bdfcb
to
8dc7fee
Compare
@bdunne updated spec test as per your comments. |
Checked commit h-kataria@8dc7fee with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
app/controllers/ops_controller/settings/common.rb
|
Fixed code that displays values saved for NTP Servers for selected zone (cherry picked from commit 77424ab) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1554825
Gaprindashvili backport details:
|
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553771
before:
after:
@bdunne @chrisarcand please review, fixed to display NTP servers values for a Zone, this issue was introduced by changes in following 2 PRs
#2720
#2359 (fixes saving of NTP servers from UI)
@dclarizio please review.