-
Notifications
You must be signed in to change notification settings - Fork 130
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
add foreman_proxy::plugin::remote_execution::ssh::ssh_log_level param #739
Conversation
baa5ed9
to
49da26e
Compare
Rebased on current master. |
I wanted to raise awareness of changes pending over here -- #734 that will affect this. |
49da26e
to
b992ab7
Compare
types/sshloglevel.pp
Outdated
@@ -0,0 +1,3 @@ | |||
# The possible openssh LogLevel values. Note that these are values allowed by the | |||
# smart_proxy_remote_execution_ssh gem and are not consistent with the ssh_config(5) man page. | |||
type foreman_proxy::sshloglevel = enum['debug', 'info', 'error', 'fatal'] |
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 is the benefit of abstracting these values to a types file versus just defining an enum directly in the 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.
DRYing the code (which isn't an issue here), readability in that the param declaration is better self documenting, and better documentation in that there are docs for the enum that don't have to be part of the param's doc string.
@@ -4,6 +4,9 @@ | |||
:local_working_dir: <%= scope.lookupvar('::foreman_proxy::plugin::remote_execution::ssh::local_working_dir') %> | |||
:remote_working_dir: <%= scope.lookupvar('::foreman_proxy::plugin::remote_execution::ssh::remote_working_dir') %> | |||
:kerberos_auth: <%= scope.lookupvar('::foreman_proxy::plugin::remote_execution::ssh::ssh_kerberos_auth') %> | |||
<% ssh_log_level = scope.lookupvar('::foreman_proxy::plugin::remote_execution::ssh::ssh_log_level'); if ssh_log_level -%> | |||
:ssh_log_level: <%= ssh_log_level %> | |||
<% end -%> |
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.
Curious your thoughts on the if
inclusion here versus setting a default value to the default from the plugin (:error
) ?
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 have a mild preference for unset parameters to not appear in the config file. At the time, I was planning to rework the gem to allow the ssh_config(5)
values to be passed directly through. The gem doesn't allow DEBUG3
to be set and there is a check that prevents :ssh_log_level: debug
from being used unless the entire foreman-proxy is also set to DEBUG
. Now that it sounds like serious work in that gem is imminent I'm not sure it is worth the effort.
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 have a mild preference for unset parameters to not appear in the config file
Sorry, what I was getting at is why not set it to :error
rather than undef
.
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.
That is the question I answered. The gem already has a hard coded default value for when the configuration isn't present in the foreman-proxy plugin config file.
b992ab7
to
749117d
Compare
7e2f30c
to
f5a8a71
Compare
Sorry for the noise, I made a booboo when converting the allowed values from uppercase to lowercase. |
@@ -0,0 +1,4 @@ | |||
include foreman_proxy | |||
class { 'foreman_proxy::plugin::remote_execution::ssh': | |||
ssh_log_level => 'DEBUG', |
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.
ssh_log_level => 'DEBUG', | |
ssh_log_level => 'debug', |
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.
🤦
f5a8a71
to
a28aef6
Compare
No description provided.