ShellScripHandler can't find the user name I specified in config file

[EasonLiao]

I wrote the username that should be used for ssh in config file like link

But ShellScriptHandler failed to use it because it's overwritten by the default parameter None in the constructor of ShellScriptHandler, see:

class ShellScriptHandler(BaseHandler):
    """ Shell Script Handler piggybacking on paramiko SSH"""

    def __init__(self, config_dir, hostname, logger_instance=None, port=22,
                 username=None, password=None, pkey=None, key_filename=None,
                 connect_timeout=None, allow_agent=True, look_for_keys=True,
                 compress=False, sock=None, verbose=False, debug=False,
                 command_timeout=None):

And I see the username and a bunch of other stuff are never passed to ShellScriptHandler, see link:

     handler = handler_class(self.config_dir, target,
                                self.logger_instance, verbose=True)

I added some ad-hoc fixes, I'm wondering what are appropriate ways to fix it? I'm happy to send a PR if you think this is something that needs to be fixed.

[sarathsreedharan]

Hey Eason,
I see that the configs are being read in the constructor
'''

    # read in yaml configuration
    for key, val in self.config.iteritems():
        setattr(self, key, val)
    del self.config

'''
unfortunately its being rewritten by the default values, so in order to fix it all you would need to do, would be to update the initialization section to update config values only if the value being passed is not None

[EasonLiao]

Hey @sarathsreedharan

Thanks for the quick reply!

I sent a PR for the fix, but unlike the proposal you suggested, I got rid of all the parameters that are related to ssh from the constructor and assumed these will be specified in config file because I don't see any reason to keep them in the constructor given that we never pass any arguments to them.

Does that look good to you? if not, I'm happy to switch to the approach you suggested :)

[sarathsreedharan]

Hey @EasonLiao
The change looks good to me, but I would also like to take @MayureshGharat and Tofig's opinion on this change, to make sure there wasn't any specific reason for putting those default parameters.