-
Notifications
You must be signed in to change notification settings - Fork 95
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 #15179 - Select box for settings #189
Conversation
The commit message is good, @theforeman-bot seems to be looking at the older commit. |
'sudo', | ||
'remote_execution_effective_user_method', | ||
nil, | ||
{ :collection => Hash[SSHExecutionProvider::EFFECTIVE_USER_METHODS.map{|method| [method, method]}]}), |
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.
This needs to be a proc, as Foreman changed how this worked shortly after, see theforeman/foreman@1f82328
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.
Using a proc for that seems pointless? It's not like it's going to change dynamically like Organisations/Locations or templates.
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.
It's explained in the issue http://projects.theforeman.org/issues/14963. This particular case may be static, but everything needs to be wrapped in proc, your change doesn't work against the latest foreman develop. It generates an exception when you load the page.
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.
I see what you mean, I've just rebased my dev environment 👍
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.
Thanks, looks good. The proc is somewhat useless here I agree, I wish it was optional but oh well.
Thanks @sean797 ! |
On 1.13 foreman_tasks needs to be initialized before :finisher_hook, since doing it 'after' has been deprecated for a while and will be removed.
Requires theforeman/foreman@ad603e4