Don't show traceback when ipa config file is not an absolute path #115
Conversation
| @@ -518,7 +518,7 @@ def build_global_parser(self, parser=None, context=None): | |||
| help='Set environment variable KEY to VAL', | |||
| ) | |||
| parser.add_option('-c', dest='conf', metavar='FILE', | |||
| help='Load configuration from FILE', | |||
| help='Load configuration from FILE. This must be an absolute path', | |||
There was a problem hiding this comment.
PEP8 check fails for this line, but it fits the code style of the surrounding code. IMO this should be fixed as a separate issue.
There was a problem hiding this comment.
you can't win this time :)
There was a problem hiding this comment.
Here should be validator for:
- file exists
- filename is absolute path
| self._merge_from_file(self.conf_default) | ||
| except ValueError as e: | ||
| root_logger.error(e) | ||
| sys.exit(1) |
There was a problem hiding this comment.
No sys.exit() in modules ever
| @@ -532,8 +533,12 @@ def _finalize_core(self, **defaults): | |||
|
|
|||
| # Merge in context config file and then default config file: | |||
| if self.__d.get('mode', None) != 'dummy': | |||
| self._merge_from_file(self.conf) | |||
| self._merge_from_file(self.conf_default) | |||
There was a problem hiding this comment.
I'd leave this failing with traceback, validation of input should happen in different place
| @@ -518,7 +518,7 @@ def build_global_parser(self, parser=None, context=None): | |||
| help='Set environment variable KEY to VAL', | |||
| ) | |||
| parser.add_option('-c', dest='conf', metavar='FILE', | |||
| help='Load configuration from FILE', | |||
| help='Load configuration from FILE. This must be an absolute path', | |||
There was a problem hiding this comment.
Here should be validator for:
- file exists
- filename is absolute path
|
NACK, please see my inline comments |
| help='Load configuration from FILE', | ||
| parser.add_option('-c', dest='conf', metavar='FILE', action='callback', | ||
| callback=config_file_callback, type='string', | ||
| help='Load configuration from FILE. This must be an absolute path', |
There was a problem hiding this comment.
IMO the callback you added creates abs path, so help should not mention 'absolute path'
Also help strings should be internationalized, I opened ticket for it https://fedorahosted.org/freeipa/ticket/6361
| value = os.path.abspath(value) | ||
| if not os.path.isfile(value): | ||
| raise optparse.OptionValueError( | ||
| "%s option '%s' is not a file" % (opt, value)) |
There was a problem hiding this comment.
-
Please use internationalized string here
-
instead of path.isfile(), I would use ipautil.file_exists() to be consistent with other parts
-
I don't like the error message, "-c option '/tmp/test.conf' is not a file" doesn't sound nice to me
IMO "%(filename)s: file not found" is better
|
NACK, please see inline comments |
|
Why the file must be absolute? I would rather remove this requirement and be done with it. |
Remove unnecessary check for absolute file paths for config file. https://fedorahosted.org/freeipa/ticket/6114
|
I found no reason why the path should be absolute, so I removed that constraint. The parser check to verify if file exists should remain, since non-existent files are otherwise ignored without any notice. |
| def config_file_callback(option, opt, value, parser): | ||
| if not ipautil.file_exists(value): | ||
| raise optparse.OptionValueError( | ||
| _("%s: file not found") % value) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Shouldn't be used this rather parser.error() instead of raising an exception? https://docs.python.org/2/library/optparse.html#how-optparse-handles-errors
|
nack, please see comments |
Add a parser check to verify config file supplied to the ipa command exists. Previously, invalid file paths would not results in any error and would just silently proceed with default config. https://fedorahosted.org/freeipa/ticket/6114
MartinBasti
added
ack
When using the ipa command with the '-c' flag, the user provides
a configuration file. If this path is not absolute, an error
without a traceback should be displayed.
https://fedorahosted.org/freeipa/ticket/6114