Change OSB Config directory from ~/.benchmark to ~/.osb#732
Change OSB Config directory from ~/.benchmark to ~/.osb#732OVI3D0 merged 7 commits intoopensearch-project:mainfrom
~/.benchmark to ~/.osb#732Conversation
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
…r message Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
OVI3D0
requested review from
IanHoang,
VijayanB,
beaioun,
gkamat and
rishabh6788
as code owners
osbenchmark/paths.py
Outdated
|
|
||
| return final_path | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Do you recall what the exact exception was? Let's try to use that one instead of a general Exception.
There was a problem hiding this comment.
It ended up being a FileNotFoundError, but not sure if it's worth changing to this exception since the latest commit includes a fix for that error. Lmk what you think though
There was a problem hiding this comment.
Even if the FileNotFoundError was addressed with the fix to the Config class, we should still incorporate it as it's an error that happened and potentially could still occur in the future.
Also, it's best to not use the base Exception class since it technically violates the principle of least astonishment. Python's error handling is designed to be specific and meaningful and using base Exception goes against this design principle
osbenchmark/paths.py
Outdated
| # Ensure symlink from .osb to .benchmark | ||
| ensure_symlink(old_path, new_path) | ||
|
|
||
| final_path = os.path.join(os.getenv("BENCHMARK_HOME", default_home), ".osb") |
There was a problem hiding this comment.
Nit: could be more descriptive with the variable name, would rename final_path to benchmark_confdir_path.
osbenchmark/paths.py
Outdated
| f"Error type: {type(e).__name__}\n" | ||
| f"Error message: {str(e)}\n" | ||
| f"Current user: {os.getlogin()}\n" | ||
| f"Current working directory: {os.getcwd()}\n" | ||
| f"Python version: {sys.version}\n" | ||
| f"Operating system: {sys.platform}\n" | ||
| f"Permissions of {old_path}: {oct(os.stat(old_path).st_mode) if os.path.exists(old_path) else 'N/A'}\n" | ||
| f"Permissions of parent of {new_path}: {oct(os.stat(os.path.dirname(new_path)).st_mode)}" |
There was a problem hiding this comment.
Since these were primarily used for debugging integ tests, we can remove them
osbenchmark/paths.py
Outdated
| f"Permissions of {old_path}: {oct(os.stat(old_path).st_mode) if os.path.exists(old_path) else 'N/A'}\n" | ||
| f"Permissions of parent of {new_path}: {oct(os.stat(os.path.dirname(new_path)).st_mode)}" | ||
| ) | ||
| print(error_message) |
There was a problem hiding this comment.
Let's use console.print from utils?
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
Signed-off-by: Michael Oviedo <mikeovi@amazon.com>
2 participants
Description
Updates OSB's config directory to ~/.osb instead of ~/.benchmark. Also adds a symlink if the old path exists but the new one does not. 'benchmark' is an overloaded term, while using ~/.osb reduces ambiguity and is more consistent with the project's name.
Issues Resolved
#728
Testing
make test+make itBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.