-
Notifications
You must be signed in to change notification settings - Fork 985
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
fixes #10832 - separating lookup keys into puppet and variable #2466
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,24 +5,24 @@ class LookupKeysController < ApplicationController | |
|
||
def index | ||
@lookup_keys = resource_base.search_for(params[:search], :order => params[:order]) | ||
.includes(:param_classes) | ||
.includes(:puppetclass) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Override the method if you want in the inherited controllers, I guess it's now removed because this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
.paginate(:page => params[:page]) | ||
@puppetclass_authorizer = Authorizer.new(User.current, :collection => @lookup_keys.map(&:puppetclass_id).compact.uniq) | ||
@puppetclass_authorizer = Authorizer.new(User.current, :collection => @lookup_keys.pluck(:puppetclass_id).compact.uniq) | ||
end | ||
|
||
def edit | ||
end | ||
|
||
def update | ||
if @lookup_key.update_attributes(params[:lookup_key]) | ||
if resource.update_attributes(params[resource_name]) | ||
process_success | ||
else | ||
process_error | ||
end | ||
end | ||
|
||
def destroy | ||
if @lookup_key.destroy | ||
if resource.destroy | ||
process_success | ||
else | ||
process_error | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
class PuppetclassLookupKeysController < LookupKeysController | ||
before_filter :setup_search_options, :only => :index | ||
|
||
def index | ||
@lookup_keys = resource_base.search_for(params[:search], :order => params[:order]) | ||
.paginate(:page => params[:page]) | ||
.includes(:param_classes) | ||
@puppetclass_authorizer = Authorizer.new(User.current, :collection => @lookup_keys.pluck(:puppetclass_id).compact.uniq) | ||
end | ||
|
||
def resource | ||
@puppetclass_lookup_key | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class VariableLookupKeysController < LookupKeysController | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where are these new controllers used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when you have a form_for object, it points to the correct controller, thus, the update action on VarLookupKeys will be called. Basically anything which uses rails' helpers for the object passed to them. |
||
def resource | ||
@variable_lookup_key | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,8 +103,8 @@ def realm_field(f, can_override = false, override = false) | |
def interesting_klasses(obj) | ||
classes = obj.all_puppetclasses | ||
classes_ids = classes.reorder('').pluck('puppetclasses.id') | ||
smart_vars = LookupKey.reorder('').where(:puppetclass_id => classes_ids).uniq.pluck(:puppetclass_id) | ||
class_vars = LookupKey.reorder('').joins(:environment_classes).where(:environment_classes => { :puppetclass_id => classes_ids }).uniq.pluck('environment_classes.puppetclass_id') | ||
smart_vars = VariableLookupKey.reorder('').where(:puppetclass_id => classes_ids).uniq.pluck(:puppetclass_id) | ||
class_vars = PuppetclassLookupKey.reorder('').joins(:environment_classes).where(:environment_classes => { :puppetclass_id => classes_ids }).uniq.pluck('environment_classes.puppetclass_id') | ||
klasses = (smart_vars + class_vars).uniq | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just wondering, anyone compared doing an inner query vs pulling it into an array? - when this was written inner queries in rails were not supported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't see a reason to do it here. I can open a new task if you think it is important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it could have an effect on large data set, if you could give it a try, maybe with the context of #2664 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, but this isn't in this PR's scope, and I don't want to fiddle with it on #2664 until this gets merged. |
||
|
||
classes.where(:id => klasses) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,19 @@ def update_counter_caches | |
if key =~ /_id/ | ||
association = self.association(key.sub(/_id$/, '').to_sym) | ||
if association.options[ :counter_cache ] | ||
counter_name = self.class.name.underscore.split("/")[0].pluralize.to_sym | ||
association.klass.reset_counters(old_value, counter_name) if old_value | ||
association.klass.reset_counters(new_value, counter_name) if new_value | ||
|
||
# in case of counter cache on STI, specify the :inverse_of option, otherwise, we might throw an error. | ||
# i.e. VariableLookupKey belongs to puppetclass, but the association name on the puppetclass is lookup_keys, not puppetclass_lookup_keys | ||
# thus, we define the correct name for the association to be derived. | ||
if association.options[:inverse_of].is_a?(Symbol) | ||
counter_name = association.options[:inverse_of] | ||
else | ||
counter_name = self.class.name.underscore.split("/")[0].pluralize.to_sym | ||
end | ||
|
||
association_name = counter_name.to_s.sub(/_count$/, "").to_sym | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry 🙇 , I can't figure out right now what made you add this? A comment here would be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in case of counter cache on STI, the counter name isn't always the class name. if that's the case, we need to use the inverse_of option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add that in the code vs in the pr? ideally with a real example :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, do we need it post rails4 upgrade? if not add a todo :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't know about rails 4, I can test it and see. you want me to put a comment in the code explaining this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. example would be great. |
||
association.klass.reset_counters(old_value, association_name) if old_value | ||
association.klass.reset_counters(new_value, association_name) if new_value | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,42 +1,52 @@ | ||
class EnvironmentClass < ActiveRecord::Base | ||
belongs_to :environment | ||
belongs_to :puppetclass | ||
belongs_to :lookup_key | ||
validates :lookup_key_id, :uniqueness => {:scope => [:environment_id, :puppetclass_id]} | ||
belongs_to :puppetclass_lookup_key | ||
validates :puppetclass_lookup_key_id, :uniqueness => {:scope => [:environment_id, :puppetclass_id]} | ||
validates :puppetclass_id, :environment_id, :presence => true | ||
|
||
scope :parameters_for_class, lambda {|puppetclasses_ids, environment_id| | ||
all_parameters_for_class(puppetclasses_ids, environment_id).where(:lookup_keys => {:override => true}) | ||
all_parameters_for_class(puppetclasses_ids, environment_id).where(:puppetclass_lookup_keys => {:override => true}) | ||
} | ||
|
||
scope :all_parameters_for_class, lambda {|puppetclasses_ids, environment_id| | ||
where(:puppetclass_id => puppetclasses_ids, :environment_id => environment_id). | ||
where('lookup_key_id is NOT NULL'). | ||
includes(:lookup_key) | ||
where('puppetclass_lookup_key_id is NOT NULL'). | ||
includes(:puppetclass_lookup_key) | ||
} | ||
|
||
scope :used_by_other_environment_classes, lambda{|lookup_key_id, this_environment_class_id| | ||
where(:lookup_key_id => lookup_key_id). | ||
scope :used_by_other_environment_classes, lambda{|puppetclass_lookup_key_id, this_environment_class_id| | ||
where(:puppetclass_lookup_key_id => puppetclass_lookup_key_id). | ||
where("id != #{this_environment_class_id}") | ||
} | ||
|
||
# These counters key track of unique puppet class keys (parameters) across environments | ||
after_create do |record| | ||
Puppetclass.increment_counter(:global_class_params_count, self.puppetclass.id) unless self.lookup_key.blank? || | ||
EnvironmentClass.used_by_other_environment_classes(self.lookup_key, self.id).count > 0 | ||
Puppetclass.increment_counter(:global_class_params_count, self.puppetclass.id) unless self.puppetclass_lookup_key.blank? || | ||
EnvironmentClass.used_by_other_environment_classes(self.puppetclass_lookup_key, self.id).count > 0 | ||
end | ||
|
||
after_destroy do |record| | ||
Puppetclass.decrement_counter(:global_class_params_count, self.puppetclass.id) unless self.lookup_key.blank? || | ||
EnvironmentClass.used_by_other_environment_classes(self.lookup_key, self.id).count > 0 | ||
Puppetclass.decrement_counter(:global_class_params_count, self.puppetclass.id) unless self.puppetclass_lookup_key.blank? || | ||
EnvironmentClass.used_by_other_environment_classes(self.puppetclass_lookup_key, self.id).count > 0 | ||
end | ||
|
||
def lookup_key_id=(val) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a deprecation warning for these two methods. I'm guessing you provide this interface to stay compatible with plugins, or is there any code in core using these methods? |
||
Foreman::Deprecation.deprecation_warning("1.12", "lookup_key_id= is deprecated, please use puppetclass_lookup_key_id= instead.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this effect api v1? should we create a setter for it there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't affect v1, will simply show the warning. you want to rename the parameter so v1 will keep on working after deprecation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add a setter/getter on API v1 as Ohad said when the deprecation is removed, adding it now is kind of pointless I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
self.puppetclass_lookup_key_id=val | ||
end | ||
|
||
def lookup_key=(val) | ||
Foreman::Deprecation.deprecation_warning("1.12", "lookup_key= is deprecated, please use puppetclass_lookup_key= instead.") | ||
self.puppetclass_lookup_key=val | ||
end | ||
|
||
#TODO move these into scopes? | ||
def self.is_in_any_environment(puppetclass, lookup_key) | ||
EnvironmentClass.where(:puppetclass_id => puppetclass, :lookup_key_id => lookup_key ).count > 0 | ||
def self.is_in_any_environment(puppetclass, puppetclass_lookup_key) | ||
EnvironmentClass.where(:puppetclass_id => puppetclass, :puppetclass_lookup_key_id => puppetclass_lookup_key ).count > 0 | ||
end | ||
|
||
def self.key_in_environment(env, puppetclass, lookup_key) | ||
EnvironmentClass.where(:environment_id => env, :puppetclass_id => puppetclass, :lookup_key_id => lookup_key ).first | ||
def self.key_in_environment(env, puppetclass, puppetclass_lookup_key) | ||
EnvironmentClass.where(:environment_id => env, :puppetclass_id => puppetclass, :puppetclass_lookup_key_id => puppetclass_lookup_key ).first | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,65 +14,39 @@ class LookupKey < ActiveRecord::Base | |
|
||
serialize :default_value | ||
|
||
belongs_to :puppetclass, :inverse_of => :lookup_keys, :counter_cache => true | ||
has_many :environment_classes, :dependent => :destroy | ||
has_many :environments, :through => :environment_classes, :uniq => true | ||
has_many :param_classes, :through => :environment_classes, :source => :puppetclass | ||
def param_class | ||
param_classes.first | ||
end | ||
|
||
def audit_class | ||
param_class || puppetclass | ||
end | ||
|
||
has_many :lookup_values, :dependent => :destroy, :inverse_of => :lookup_key | ||
accepts_nested_attributes_for :lookup_values, | ||
:reject_if => :reject_invalid_lookup_values, | ||
:allow_destroy => true | ||
|
||
before_validation :cast_default_value, :unless => Proc.new{|p| p.use_puppet_default } | ||
validates :key, :uniqueness => {:scope => :is_param }, :unless => Proc.new{|p| p.is_param?} | ||
before_validation :cast_default_value | ||
|
||
validates :key, :presence => true | ||
validates :puppetclass, :presence => true, :unless => Proc.new {|k| k.is_param?} | ||
validates :validator_type, :inclusion => { :in => VALIDATOR_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true | ||
validates :key_type, :inclusion => {:in => KEY_TYPES, :message => N_("invalid")}, :allow_blank => true, :allow_nil => true | ||
validate :validate_default_value | ||
validates_associated :lookup_values | ||
validate :ensure_type, :disable_merge_overrides, :disable_avoid_duplicates, :disable_merge_default | ||
validate :disable_merge_overrides, :disable_avoid_duplicates, :disable_merge_default | ||
|
||
before_save :sanitize_path | ||
attr_name :key | ||
|
||
scoped_search :on => :key, :complete_value => true, :default_order => true | ||
scoped_search :on => :lookup_values_count, :rename => :values_count | ||
scoped_search :on => :override, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :merge_overrides, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :merge_default, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :avoid_duplicates, :complete_value => {:true => true, :false => false} | ||
scoped_search :in => :param_classes, :on => :name, :rename => :puppetclass, :complete_value => true | ||
scoped_search :in => :lookup_values, :on => :value, :rename => :value, :complete_value => true | ||
def self.inherited(child) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh: I'm not sure why this is needed to be eval on the child, if its a common attribute its fine to leave it in the base class, then specific cases can go to the sub class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wvanbergen/scoped_search#112 (comment) |
||
child.instance_eval do | ||
scoped_search :on => :key, :complete_value => true, :default_order => true | ||
scoped_search :on => :lookup_values_count, :rename => :values_count | ||
scoped_search :on => :override, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :merge_overrides, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :merge_default, :complete_value => {:true => true, :false => false} | ||
scoped_search :on => :avoid_duplicates, :complete_value => {:true => true, :false => false} | ||
scoped_search :in => :lookup_values, :on => :value, :rename => :value, :complete_value => true | ||
end | ||
end | ||
|
||
default_scope -> { order('lookup_keys.key') } | ||
|
||
scope :override, -> { where(:override => true) } | ||
|
||
scope :smart_class_parameters_for_class, lambda { |puppetclass_ids, environment_id| | ||
joins(:environment_classes).where(:environment_classes => {:puppetclass_id => puppetclass_ids, :environment_id => environment_id}) | ||
} | ||
|
||
scope :parameters_for_class, lambda { |puppetclass_ids, environment_id| | ||
override.smart_class_parameters_for_class(puppetclass_ids,environment_id) | ||
} | ||
|
||
scope :global_parameters_for_class, lambda { |puppetclass_ids| | ||
where(:puppetclass_id => puppetclass_ids) | ||
} | ||
|
||
scope :smart_variables, -> { where('lookup_keys.puppetclass_id > 0').readonly(false) } | ||
scope :smart_class_parameters, -> { where(:is_param => true).joins(:environment_classes).readonly(false) } | ||
|
||
# new methods for API instead of revealing db names | ||
alias_attribute :parameter, :key | ||
alias_attribute :variable, :key | ||
|
@@ -88,16 +62,12 @@ def self.find_by_name(str) | |
nil | ||
end | ||
|
||
def to_label | ||
"#{audit_class}::#{key}" | ||
end | ||
|
||
def is_smart_variable? | ||
puppetclass_id.to_i > 0 | ||
def audit_class | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing whitespace between methods. |
||
self | ||
end | ||
|
||
def is_smart_class_parameter? | ||
is_param? && environment_classes.any? | ||
def to_label | ||
"#{audit_class}::#{key}" | ||
end | ||
|
||
def supports_merge? | ||
|
@@ -172,6 +142,10 @@ def overridden?(host) | |
lookup_values.find_by_match(host.send(:lookup_value_match)).present? | ||
end | ||
|
||
def puppet? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we change this to something like features? this will not require the parent class to know anything about the child class and you could ask each class for its "features or capabilities" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like a massive overkill... this is needed because of legacy, no need for any other feature to be added at this time, why complicate this? parent class can definitely define smart defaults, like in here, the default is that LK is not related to a puppetclass, only in instances of PuppetclassLK we will override this method to be true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still feel wrong to me, other opinions ? |
||
false | ||
end | ||
|
||
private | ||
|
||
# Generate possible lookup values type matches to a given host | ||
|
@@ -232,12 +206,6 @@ def load_yaml_or_json(value) | |
end | ||
end | ||
|
||
def ensure_type | ||
if puppetclass_id.present? and is_param? | ||
self.errors.add(:base, _('Global variable or class Parameter, not both')) | ||
end | ||
end | ||
|
||
def validate_default_value | ||
Foreman::Parameters::Validator.new(self, | ||
:type => validator_type, | ||
|
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.
why not search on the base class? or search on the base class and limit the type instead of to_a and + ?
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 need both types, searching the base class didn't work because we moved scoped search out of the base class since there was a problem with scoped search that the first class it sees it uses only that.
As a result defining some things to search in the base class and others in
VariableLookupKey
causes an error when searching on what was defined in the child classEdit: existing but in scoped search: wvanbergen/scoped_search#112
Currently we can workaround this by adding all the relationships to the base class.
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.
what happens if the user passes an invalid search item for one of the lookup keys sub classes? imho it would break?
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.
right now it works since the only thing you can search that isn't shared is puppetclass and paramclass.
paramclass id defined in both and puppetclass works on puppetclass_lookupkey probably since it exists through environment_classes.
It could become a problem so I would open a refactoring issue and maybe the bug in scoped_search will be fixed by then. Otherwise we can also put all the association in the parent which means the classes still aren't completely separated.