fixes #10832 - separating lookup keys into puppet and variable #2466
Conversation
| @@ -13,4 +13,6 @@ | |||
| gem 'factory_girl_rails', '~> 4.5', :require => false | |||
| gem 'rubocop-checkstyle_formatter', '~> 0.1' | |||
| gem "poltergeist" | |||
| gem "pry" | |||
| @@ -0,0 +1,7 @@ | |||
| require 'test_helper' | |||
There was a problem hiding this comment.
These files (the empty tests) don't really serve any purpose, we can do without them for the moment.
unorthodoxgeek
force-pushed
the
10832
branch
2 times, most recently
from
0390fee
to
61c4509
Compare
| @@ -34,4 +33,14 @@ def destroy | |||
| def controller_permission | |||
| 'external_variables' | |||
| end | |||
|
|
|||
| def find_resource | |||
There was a problem hiding this comment.
This is defining both @lookup_key and @variable_lookup_key or @puppetclass_lookup_key. How about overriding resource_name so that it's always "lookup_key"?
dLobatog
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@elobato please re review - I think I've applied all changes you requested 😄 |
| } | ||
|
|
||
| def lookup_key_id=(val) | ||
| ::ActiveSupport::Deprecation.warn('lookup_key_id= is deprecated, please use puppetclass_lookup_key_id= instead.') |
| end | ||
|
|
||
| def lookup_key_id=(val) | ||
| 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.
does this effect api v1? should we create a setter for it there?
There was a problem hiding this comment.
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.
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.
holy molly! Daniel agreed with me!
unorthodoxgeek
force-pushed
the
10832
branch
2 times, most recently
from
1038253
to
1fe7b8b
Compare
unorthodoxgeek
force-pushed
the
10832
branch
from
3bdd370
to
d699b2e
Compare
| @@ -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')) | |||
There was a problem hiding this comment.
This error was not only confusing but I think it only shows up in the logs, not the UI on the develop branch now. Great to see it going away.
|
👍 with me, let's give this a grace period of one day due to the huge size of changes. I'll also take more time to play with it tomorrow and see if I missed anything. Thanks @orrabin & @unorthodoxgeek |
| .includes(:param_classes) | ||
| .paginate(:page => params[:page]) | ||
| @lookup_keys = resource_base.search_for(params[:search], :order => params[:order]).includes(:puppetclass) | ||
| .paginate(:page => params[:page]) |
There was a problem hiding this comment.
nitpick: identation looks off? maybe keep the additional methods per line as it was before.
|
@orrabin @unorthodoxgeek Needs rebasing now, sorry! |
| @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.map(&:puppetclass_id).compact.uniq) |
There was a problem hiding this comment.
nitpick: use pluck instead of map?
|
@elobato rebased and changed map to pluck in lookup_key_controller and puppetclass_lookup_key_controller |
unorthodoxgeek
force-pushed
the
10832
branch
from
69e40aa
to
372e9f0
Compare
|
Alright, grace period ended. I gave it a last round of review & tested it again, feels fairly solid. I'm guessing it'll contain some unforeseen bug, but we have plenty of time before 1.10 to iron out the issues. |
|
@unorthodoxgeek @orrabin The manual should contain updated versions of Parametrized Classes and Smart Variables to reflect the changes. |
|
Merged as ff7ae22, thanks @unorthodoxgeek! |
this PR is a combination @orrabin's and my work
@elobato 's edit - a bit more context https://dl.dropboxusercontent.com/u/5892944/Foreman-Parameters-2015-04-16.pdf