-
Notifications
You must be signed in to change notification settings - Fork 983
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 #16578 - make keep_param idempotent, remove duplicate call #3853
Conversation
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.
Just a small comment to explain what keep_params is doing and why - otherwise 👍 , thanks for fixing this @domcleal
@@ -9,6 +9,8 @@ def keep_param(params, top_level_hash, *keys) | |||
old_params = keys.inject({}) do |op,(key,val)| | |||
params[top_level_hash].has_key?(key) ? op.update(key => params[top_level_hash].delete(key)) : op | |||
end | |||
yield.update old_params | |||
filtered = yield.update(old_params) | |||
params[top_level_hash].update old_params |
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.
Please leave a comment here explaining this restores deleted params
, I don't think it's very clear at first glance
Using host_params (and keep_param) twice was causing compute_attributes to be deleted permanently out of `params`. keep_param now restores elements it deletes, and the controller now only calls host_params once for efficiency.
6025525
to
130c1c8
Compare
@dLobatog comments added for both restoration lines, hopefully it's clearer now. |
Ready to merge if Jenkins tests pass |
[test foreman] |
[test foreman] as develop changed. |
Thanks @domcleal ! |
Using host_params (and keep_param) twice was causing compute_attributes
to be deleted permanently out of
params
. keep_param now restoreselements it deletes, and the controller now only calls host_params once
for efficiency.