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

Fixed code that displays values saved for NTP Servers for selected zone #3568

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

h-kataria
Copy link
Contributor

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

before:
before

after:
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.

Copy link
Member

@bdunne bdunne left a 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)
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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"]}})
Copy link
Member

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.

@h-kataria h-kataria force-pushed the display_ntp_servers_for_zone branch from d7bdfcb to 8dc7fee Compare March 12, 2018 17:07
@h-kataria
Copy link
Contributor Author

@bdunne updated spec test as per your comments.

@miq-bot
Copy link
Member

miq-bot commented Mar 12, 2018

Checked commit h-kataria@8dc7fee with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

app/controllers/ops_controller/settings/common.rb

@dclarizio dclarizio merged commit 77424ab into ManageIQ:master Mar 12, 2018
@dclarizio dclarizio added this to the Sprint 81 Ending Mar 12, 2018 milestone Mar 12, 2018
@bdunne bdunne mentioned this pull request Mar 12, 2018
simaishi pushed a commit that referenced this pull request Mar 13, 2018
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
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit c03877502d8477790f4551a4f24ceb6f406a482f
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Mon Mar 12 10:51:13 2018 -0700

    Merge pull request #3568 from h-kataria/display_ntp_servers_for_zone
    
    Fixed code that displays values saved for NTP Servers for selected zone
    (cherry picked from commit 77424ab866c41995f6ede8b6260726cc71fb1a63)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1554825

@h-kataria h-kataria deleted the display_ntp_servers_for_zone branch March 14, 2018 20:18
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