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

Ability to reset settings to default value and delete newly added keys #17482

Merged
merged 6 commits into from
Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion lib/vmdb/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
module Vmdb
class Settings
extend Vmdb::SettingsWalker::ClassMethods
include Vmdb::Logging

class ConfigurationInvalid < StandardError
attr_accessor :errors
Expand All @@ -21,6 +22,11 @@ def initialize(errors)
PASSWORD_FIELDS = Vmdb::SettingsWalker::PASSWORD_FIELDS
DUMP_LOG_FILE = Rails.root.join("log/last_settings.txt").freeze

# RESET_COMMAND remove record for selected key and specific resource
# RESET_ALL_COMMAND remove all records for selected key and all resources
Copy link
Member

Choose a reason for hiding this comment

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

I think these should match the name of the constant, right? DELETE_COMMAND and DELETE_ALL_COMMAND

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

MAGIC_VALUES = [DELETE_COMMAND = "<<reset>>".freeze,
DELETE_ALL_COMMAND = "<<reset_all>>".freeze].freeze

cattr_accessor :last_loaded

def self.init
Expand Down Expand Up @@ -62,8 +68,9 @@ def self.valid?

def self.save!(resource, hash)
new_settings = build_without_local(resource).load!.merge!(hash).to_hash
settings_to_validate = remove_magic_values(new_settings.deep_clone)

valid, errors = validate(new_settings)
valid, errors = validate(settings_to_validate)
raise ConfigurationInvalid.new(errors) unless valid # rubocop:disable Style/RaiseArgs

hash_for_parent = parent_settings_without_local(resource).load!.to_hash
Expand Down Expand Up @@ -188,6 +195,7 @@ def self.apply_settings_changes(resource, deltas)

deltas.each do |delta|
record = index.delete(delta[:key])
next if process_magic_value(resource, delta)
if record
record.update_attributes!(delta)
else
Expand All @@ -198,5 +206,34 @@ def self.apply_settings_changes(resource, deltas)
end
end
private_class_method :apply_settings_changes

def self.process_magic_value(resource, delta)
return false unless MAGIC_VALUES.include?(delta[:value])
case delta[:value]
when DELETE_COMMAND
_log.info("removing custom settings #{delta[:key]} on #{resource.class} id:#{resource.id}")
Copy link
Member

Choose a reason for hiding this comment

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

I think this should read resetting ... to be consistent with <reset_all>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

resource.settings_changes.where("key LIKE ?", "#{delta[:key]}%").destroy_all
when DELETE_ALL_COMMAND
_log.info("resetting #{delta[:key]} to defaul for all resorces")
Copy link
Member

Choose a reason for hiding this comment

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

Typo s/defaul/default/

SettingsChange.where("key LIKE ?", "#{delta[:key]}%").destroy_all
end
true
end
private_class_method :process_magic_value

def self.walk_hash(hash, path = [], &block)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why we are keeping track of this path parameter here.
We are yielding it and passing it along into the next level of recursion, but don't use it in the block in .remove_magic_values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, we do not need it here if we are ignoring Arrays
I was thinking to process Arrays too (as in SettingsWalker from were this method was copied), but could not find example in our settings.yml and decided not top do so.
Removing it ...

@Fryguy should we accommodate resetting nested Hash as an element of Array in Settings ?

hash.each do |key, value|
Copy link
Member

Choose a reason for hiding this comment

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

Prefer using the existing walk method, which already handles this. Even so, I don't think Iike this manual thing at all, A reset is really just taking the parent's value at that level and replacing it. Then, let the differ handle the rest.

key_path = path.dup << key
yield key, value, key_path, hash
walk_hash(value, key_path, &block) if value&.class == hash.class
Copy link
Member

Choose a reason for hiding this comment

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

Won't hash.class just always be Hash? Also do we want to recurse into something like an array of hashes? I think this would fail in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carbonin it is always Hash, will change.
Array of Hashes would be OK, I am adding another rspec test for it ...

end
end
private_class_method :walk_hash

def self.remove_magic_values(hash)
walk_hash(hash) { |key, value, _path, owner| owner.delete(key) if MAGIC_VALUES.include?(value) }
hash
end
private_class_method :remove_magic_values
end
end
73 changes: 73 additions & 0 deletions spec/lib/vmdb/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,79 @@
miq_server.zone.reload
expect(miq_server.zone.settings_changes.count).to eq 0
end

context "deleting entries" do
let(:server_value) { 1 }
let(:zone_value) { 2 }

let(:reset) { ::Vmdb::Settings::DELETE_COMMAND }
let(:reset_all) { ::Vmdb::Settings::DELETE_ALL_COMMAND }

let(:second_server) { FactoryGirl.create(:miq_server, :zone => miq_server.zone) }

before do
MiqRegion.seed

described_class.save!(miq_server, :api => {:token_ttl => server_value}, :session => {:timeout => server_value})
described_class.save!(miq_server, :api => {:new_key => "new value"})
described_class.save!(second_server, :api => {:token_ttl => server_value}, :session => {:timeout => server_value})

described_class.save!(miq_server.zone, :api => {:token_ttl => zone_value}, :session => {:timeout => zone_value})
expect(SettingsChange.count).to eq 7
end

context "magic value <<reset>>" do
it "deletes key-value for specific key for the resource if specified on leaf level" do
described_class.save!(miq_server, :api => {:token_ttl => reset})

expect(SettingsChange.count).to eq 6
expect(miq_server.settings_changes.find_by(:key => "/api/token_ttl")).to be nil
expect(Vmdb::Settings.for_resource(miq_server).api.new_key).to eq "new value"
expect(Vmdb::Settings.for_resource(second_server).api.token_ttl).to eq server_value
end

it "deletes current node and all sub-nodes for the resource if specified on node level" do
described_class.save!(miq_server, :api => reset)

expect(SettingsChange.count).to eq 5
expect(miq_server.settings_changes.where("key LIKE ?", "/api%").count).to eq 0
expect(Vmdb::Settings.for_resource(miq_server).api.new_key).to eq nil
expect(Vmdb::Settings.for_resource(second_server).api.token_ttl).to eq server_value
end

it "deletes new key-value settings not present in defaul yaml" do
described_class.save!(miq_server, :api => {:new_key => reset})
expect(Vmdb::Settings.for_resource(miq_server).api.new_key).to eq nil
end

it "passes validation" do
described_class.save!(miq_server, :session => {:timeout => reset})
expect(Vmdb::Settings.for_resource(miq_server).session.timeout).to eq zone_value
end
end

context "magic value <<reset_all>>" do
it "deletes all key-value for specific key for all resource if specified on leaf levelher" do
described_class.save!(miq_server, :api => {:token_ttl => reset_all})

expect(SettingsChange.where("key LIKE ?", "/api/token_ttl").count).to eq 0
expect(SettingsChange.where("key LIKE ?", "/api%").count).to eq 1
expect(SettingsChange.where("key LIKE ?", "/session%").count).to eq 3
end

it "deletes specific node and all sub-nodes for all resources if specified on node level" do
described_class.save!(miq_server, :api => reset_all)

expect(SettingsChange.where("key LIKE ?", "/api%").count).to eq 0
expect(SettingsChange.where("key LIKE ?", "/session%").count).to eq 3
end

it "passes validation" do
described_class.save!(miq_server, :session => {:timeout => reset_all})
expect(SettingsChange.where("key LIKE ?", "/session%").count).to eq 0
end
end
end
end

describe "save_yaml!" do
Expand Down