Changed get_file method to receive a resource as parameter. #17406

h-kataria · 2018-05-10T18:03:21Z

This change returns settings for passed in resource region/zone/server.

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

Fryguy

Additionally, can you create a second test instead of modifying the original? The reason is because the original tests that MiqServer.my_server is used as the default, whereas this tests shows passing a resource.
Alternately, if get_file is not used anywhere else, and you are only ever going to pass a resource, then we can remove the defaulting in this method.

Fryguy · 2018-05-10T19:45:08Z

spec/lib/vmdb/config_spec.rb

- expect(VMDB::Config.get_file).to eq(
- "---\n:http_proxy:\n :default:\n :host: proxy.example.com\n :user: user\n :password: #{enc_pass}\n :port: 80\n"
+ expect(VMDB::Config.get_file(resource)).to eq(
+ "--- !ruby/object:Config::Options\ntable:\n :http_proxy: !ruby/object:Config::Options\n  table:\n :default: !ruby/object:Config::Options\n  table:\n :host: proxy.example.com\n  :user: user\n  :password: #{enc_pass}\n  :port: 80\n modifiable: true\n modifiable: true\n"


If that is the result of running your test, then something is broken, because we don't want yaml with Config::Options in it

Try .to_hash.to_yaml instead?

@Fryguy fixed that, please re-review

Fryguy · 2018-05-10T21:40:39Z

spec/lib/vmdb/config_spec.rb

 stub_settings(:http_proxy => {:default => {:host => "proxy.example.com", :user => "user", :password => password, :port => 80}})

 expect(VMDB::Config.get_file).to eq(
 "---\n:http_proxy:\n :default:\n :host: proxy.example.com\n :user: user\n :password: #{enc_pass}\n :port: 80\n"
 )
 end

+ it ".get_file for sepecified resource" do


typo sepecified -> specified

Fryguy · 2018-05-10T21:41:47Z

spec/lib/vmdb/config_spec.rb

@@ -2,14 +2,24 @@
 let(:password) { "pa$$w0rd" }
 let(:enc_pass) { MiqPassword.encrypt(password) }

- it ".get_file" do
+ it ".get_file for current server" do


Can you reorgnize this like

describe ".get_file" do it "for current server" do end it "for specified resource" do end end

Fryguy · 2018-05-10T21:50:57Z

spec/lib/vmdb/config_spec.rb

 stub_settings(:http_proxy => {:default => {:host => "proxy.example.com", :user => "user", :password => password, :port => 80}})

 expect(VMDB::Config.get_file).to eq(
 "---\n:http_proxy:\n :default:\n :host: proxy.example.com\n :user: user\n :password: #{enc_pass}\n :port: 80\n"
 )
 end

+ it ".get_file for sepecified resource" do
+ resource = FactoryGirl.create(:miq_server)
+ stub_settings(:http_proxy => {:default => {:host => "proxy.example.com", :user => "user", :password => password, :port => 80}})


stub_settings should probably not be used here because the way it works is by stubbing things globally.

Instead, I think it's better to maybe create some changes, save them, then see if they come back? This should be done in both tests.

Maybe something like...

resource = FactoryGirl.create(:miq_server) Vmdb::Settings.save!(resource, {:http_proxy => {:default => {:host => "proxy.example.com", :user => "user", :password => password, :port => 80}}}) resource.reload yaml = VMDB::Config.get_file(resource) yaml = YAML.load(yaml) expect(yaml.fetch_path(:http_proxy, :default, :host)).to eq "proxy.example.com" expect(yaml.fetch_path(:http_proxy, :default, :user)).to eq "user" expect(yaml.fetch_path(:http_proxy, :default, :port)).to eq 80 expect(yaml.fetch_path(:http_proxy, :default, :password)).to be_encrypted

@h-kataria I edited the snippet above to be more proper

h-kataria · 2018-05-10T22:06:20Z

@Fryguy made requested changes

Fryguy · 2018-05-11T16:28:45Z

lib/vmdb/config.rb

- def self.get_file
- Vmdb::Settings.encrypt_passwords!(::Settings.to_hash).to_yaml
+ def self.get_file(resource = MiqServer.my_server)
+ Vmdb::Settings.encrypt_passwords!(resource.settings_for_resource).to_hash.to_yaml


@h-kataria You have to put the .to_hash inside of the encrypt_passwords, otherwise it modifies the passwords of the resource's settings).

Vmdb::Settings.encrypt_passwords!(resource.settings_for_resource.to_hash).to_yaml

Fryguy · 2018-05-11T16:29:50Z

@h-kataria I think you missed my changes as requested in #17406 (review) . The code is still using stub_settings which is not testing this properly.

h-kataria · 2018-05-11T17:14:07Z

@Fryguy updated once again!

Fryguy · 2018-05-15T17:02:29Z

@h-kataria

Fix the trailing whitespace issue from the bot
Please don't put the assertions in an after block. after blocks are meant for cleanup, and should rarely be used. Instead prefer a regular method like assert_setttings_values or something if you're trying to DRY it up.

bdunne

I think you want shared examples or a method where the expectations can live instead since the expectations shouldn't be in the after block. Then you can also expect the hash to have_attributes so that it is a single expectation.

bdunne · 2018-05-15T17:05:29Z

spec/lib/vmdb/config_spec.rb

+
+ after do
+ yaml = YAML.load(@yaml)
+ expect(yaml.fetch_path(:http_proxy, :default, :host)).to eq "proxy.example.com"


Why are the expectations in the after block? Shouldn't these be in the test or a shared example instead?

This change returns settings for passed in resource region/zone/server. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536524

miq-bot · 2018-05-15T21:46:15Z

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

spec/lib/vmdb/config_spec.rb

❗ - Line 21, Col 19 - Security/YAMLLoad - Prefer using YAML.safe_load over YAML.load.
❗ - Line 43, Col 19 - Security/YAMLLoad - Prefer using YAML.safe_load over YAML.load.

h-kataria · 2018-05-16T14:45:18Z

@Fryguy @bdunne please review. I am running into error when using safe_load instead of YAML.load in the tests, addressed all other comments. Getting following error when using YAML.safe_load

 Failure/Error: yaml = YAML.safe_load(yaml)
     
     Psych::DisallowedClass:
       Tried to load unspecified class: Symbol

Talk to him and he is good 👍

h-kataria added the enhancement label May 10, 2018

h-kataria assigned Fryguy May 10, 2018

h-kataria mentioned this pull request May 10, 2018

Made "Advanced" tab visible for all Server nodes. ManageIQ/manageiq-ui-classic#3925

Merged

h-kataria force-pushed the advanced_config_settings branch from fd864d5 to e008c16 Compare May 10, 2018 18:58

Fryguy requested changes May 10, 2018

View reviewed changes

h-kataria force-pushed the advanced_config_settings branch 2 times, most recently from ca80e4c to d5d0f09 Compare May 10, 2018 21:21

Fryguy reviewed May 10, 2018

View reviewed changes

h-kataria force-pushed the advanced_config_settings branch 2 times, most recently from a96e47f to f6331b6 Compare May 10, 2018 22:05

Fryguy reviewed May 11, 2018

View reviewed changes

h-kataria force-pushed the advanced_config_settings branch from f6331b6 to 5e943a4 Compare May 11, 2018 16:59

h-kataria force-pushed the advanced_config_settings branch 3 times, most recently from 0eda305 to 7972878 Compare May 14, 2018 22:23

bdunne previously requested changes May 15, 2018

View reviewed changes

h-kataria force-pushed the advanced_config_settings branch from 7972878 to 44c182d Compare May 15, 2018 21:37

Changed get_file method to receive a resource as parameter.

91b7996

This change returns settings for passed in resource region/zone/server. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536524

h-kataria force-pushed the advanced_config_settings branch from 44c182d to 91b7996 Compare May 15, 2018 21:45

Fryguy approved these changes May 17, 2018

View reviewed changes

Fryguy merged commit 9d5ed08 into ManageIQ:master May 17, 2018

Fryguy added this to the Sprint 86 Ending May 21, 2018 milestone May 17, 2018

h-kataria deleted the advanced_config_settings branch April 14, 2020 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed get_file method to receive a resource as parameter. #17406

Changed get_file method to receive a resource as parameter. #17406

h-kataria commented May 10, 2018

Fryguy left a comment

Fryguy May 10, 2018

Fryguy May 10, 2018 •

edited

Loading

h-kataria May 10, 2018

Fryguy May 10, 2018

Fryguy May 10, 2018

Fryguy May 10, 2018 •

edited

Loading

Fryguy May 10, 2018

h-kataria commented May 10, 2018

Fryguy May 11, 2018

Fryguy commented May 11, 2018

h-kataria commented May 11, 2018

Fryguy commented May 15, 2018

bdunne left a comment

bdunne May 15, 2018

miq-bot commented May 15, 2018

h-kataria commented May 16, 2018

Changed get_file method to receive a resource as parameter. #17406

Changed get_file method to receive a resource as parameter. #17406

Conversation

h-kataria commented May 10, 2018

Fryguy left a comment

Choose a reason for hiding this comment

Fryguy May 10, 2018

Choose a reason for hiding this comment

Fryguy May 10, 2018 • edited Loading

Choose a reason for hiding this comment

h-kataria May 10, 2018

Choose a reason for hiding this comment

Fryguy May 10, 2018

Choose a reason for hiding this comment

Fryguy May 10, 2018

Choose a reason for hiding this comment

Fryguy May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Fryguy May 10, 2018

Choose a reason for hiding this comment

h-kataria commented May 10, 2018

Fryguy May 11, 2018

Choose a reason for hiding this comment

Fryguy commented May 11, 2018

h-kataria commented May 11, 2018

Fryguy commented May 15, 2018

bdunne left a comment

Choose a reason for hiding this comment

bdunne May 15, 2018

Choose a reason for hiding this comment

miq-bot commented May 15, 2018

h-kataria commented May 16, 2018

Fryguy May 10, 2018 •

edited

Loading

Fryguy May 10, 2018 •

edited

Loading