Config error handling #301
Conversation
There was a problem hiding this comment.
Some minor comments, but hopefully should be okay even without addressing them.
I was also wondering if validate_config() (or the new equivalent) should be done in two steps: generate_ssh_keys() and then verify_config(). This way:
- The steps would be explicit rather than implicit (no surprises when a validate method attempts to write files); and
- If someone has ssh keys they've generated themselves, they can still benefit from
verify_config().
| dir_path = os.path.dirname(bastion_ssh_config_file) | ||
| if not os.path.exists(dir_path): | ||
| msg = ( | ||
| f"Directory {dir_path} does not exist, can not create ssh_config file!" | ||
| ) | ||
| print(msg) | ||
| logging.error(msg) | ||
| raise Exception(msg) |
There was a problem hiding this comment.
Perhaps we can attempt to do an os.mkdirs(dir_path) or pathlib.Path(dir_path).mkdir(parents=True) here, and then fail?
| ) | ||
| print(msg) | ||
| logging.error(msg) | ||
| raise Exception(msg) |
There was a problem hiding this comment.
Same as previous comment -- we could perhaps attempt to create the directory before failing? Would that be somehow risky?
|
Oops, this got merged when I wandered away for a few minutes. :-) |
|
@kthare10 I will file some issues and start a PR if my suggestions make sense. What do you think? |
|
Agree on attempting to create directory before failing.
|
Pull Request Test Coverage Report for Build 8557097546Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Addresses following issues:
#300
#299