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

Made "Advanced" tab visible for all Server nodes. #3925

Merged

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented May 10, 2018

Allow edit of advanced config setting for all servers. Made changes so this code can be used to edit Zone/Region config settings in future.

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

Dependent on core PR ManageIQ/manageiq#17406

before no Advanced tab on Remote server level:
before

after:
after

Allow edit of advanced config setting for all servers. Made changes so this code can be used to edit Zone/Region config settings in future.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536524
@h-kataria h-kataria force-pushed the advanced_config_for_all_appliances branch from 6c8e49d to 39f0e57 Compare May 10, 2018 19:04
@chessbyte
Copy link
Member

@h-kataria happy to see this PR. /cc @Fryguy

@h-kataria h-kataria changed the title Made "Advanced" tab visible for all Server nodes. [WIP] - Made "Advanced" tab visible for all Server nodes. May 11, 2018
@miq-bot miq-bot added the wip label May 11, 2018
@Fryguy
Copy link
Member

Fryguy commented May 14, 2018

Once the backend PR changes, please update the screenshots. As you can see, this is showing Config::Options objects and not pure yaml.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

Needs backend merged first

@h-kataria h-kataria changed the title [WIP] - Made "Advanced" tab visible for all Server nodes. Made "Advanced" tab visible for all Server nodes. May 17, 2018
@h-kataria
Copy link
Contributor Author

@Fryguy updated screenshot.

@h-kataria
Copy link
Contributor Author

@lgalis please test/review.

@lgalis
Copy link
Contributor

lgalis commented May 17, 2018

Looks good. Tested in the UI that the Advanced settings can be edited and saved for each server. The types of validation errors detected by the config validator for the current server are also detected and displayed for all other servers.


context 'get advanced config settings' do
it 'for selected server' do
_guid, miq_server, _zone = EvmSpecHelper.local_guid_miq_server_zone
Copy link
Member

Choose a reason for hiding this comment

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

To ensure that this code works for any server and not just my_server, this code should

  • create a server via Factory.create(:miq_server)
  • not use stub_settings. stub_settings cheats by hacking the ::Settings constant and also makes it looks like every server returns that value. Since you are testing settings itself, you shouldn't use it here, because it's bypassing the code you want to verify. Instead create a server object and actually save settings into it, like was done with the parent PR.


controller.send(:settings_update_save)
controller.send(:fetch_advanced_settings, miq_server)
expect(VMDB::Config.save_file(data, miq_server)).to be true
Copy link
Member

Choose a reason for hiding this comment

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

The controller itself should be calling this, so why are you calling it in the test directly?

@h-kataria h-kataria force-pushed the advanced_config_for_all_appliances branch from 367e130 to e15cd4a Compare May 18, 2018 14:10
@h-kataria
Copy link
Contributor Author

@Fryguy addressed feedback.

@Fryguy
Copy link
Member

Fryguy commented May 18, 2018

LGTM but tests are failing.

@h-kataria h-kataria force-pushed the advanced_config_for_all_appliances branch from e15cd4a to 8e27e8b Compare May 18, 2018 16:11
@h-kataria h-kataria assigned mzazrivec and unassigned dclarizio May 18, 2018
@miq-bot
Copy link
Member

miq-bot commented May 18, 2018

Checked commits h-kataria/manageiq-ui-classic@39f0e57~...8e27e8b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@h-kataria
Copy link
Contributor Author

@mzazrivec please merge

@mzazrivec mzazrivec added this to the Sprint 86 Ending May 21, 2018 milestone May 21, 2018
@mzazrivec mzazrivec merged commit 6c954d7 into ManageIQ:master May 21, 2018
@h-kataria h-kataria deleted the advanced_config_for_all_appliances branch July 23, 2018 16:07
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.

None yet

7 participants