Remove separate config for island logger #1151
Conversation
…rver_config.json
…land/cc/server_utils/test_island_logger.py)
Codecov Report
@@ Coverage Diff @@
## develop #1151 +/- ##
===========================================
+ Coverage 28.62% 28.67% +0.04%
===========================================
Files 411 412 +1
Lines 12742 12755 +13
===========================================
+ Hits 3648 3658 +10
- Misses 9094 9097 +3
Continue to review full report at Codecov.
|
shreyamalviya
requested review from
mssalvatore and
VakarisZ
and removed request for
mssalvatore
| if not os.path.exists(data_dir_path): | ||
| os.makedirs(data_dir_path, mode=0o700, exist_ok=True) |
There was a problem hiding this comment.
This should not be the responsibility of the logger. The logger can assume the directory it's been configured to write to exists.
| from typing import Dict | ||
|
|
||
| from monkey_island.cc.server_utils.consts import DEFAULT_LOGGER_CONFIG_PATH | ||
|
|
||
| __author__ = "Maor.Rayzin" |
There was a problem hiding this comment.
Remove this __author__ field.
monkey/monkey_island.py
Outdated
| server_config_path = os.path.expanduser(island_args.server_config) | ||
| if not Path(server_config_path).is_file(): | ||
| server_config_generator.create_default_config_file(server_config_path) |
There was a problem hiding this comment.
If the user has a typo in their command-line argument, this will use a config other than the one they specified and pretend like everything is OK. Monkey Island will run with the wrong config and the user will waste time trying to figure out why the island isn't behaving as expected.
Just read the file. Trap any OSErrors and print a useful message for the user.
| DEFAULT_LOGGER_CONFIG_PATH = os.path.join( | ||
| MONKEY_ISLAND_ABS_PATH, "cc", "island_logger_default_config.json" | ||
| ) | ||
| DEFAULT_LOG_LEVEL = "NOTSET" |
There was a problem hiding this comment.
Why not DEFAULT or INFO? Also, if this is only used in monkey_island.py, it can probably be moved there.
monkey/monkey_island.py
Outdated
| with open(server_config_path, "r") as f: | ||
| config_content = f.read() | ||
| data = json.loads(config_content) | ||
| data_dir = os.path.abspath( | ||
| os.path.expanduser( | ||
| os.path.expandvars(data["data_dir"] if "data_dir" in data else DEFAULT_DATA_DIR) | ||
| ) | ||
| ) | ||
| log_level = data["log_level"] if "log_level" in data else DEFAULT_LOG_LEVEL |
There was a problem hiding this comment.
This can probably be a separate function called load_server_config() or something similar.
Make sure to write tests for load_server_config() that test all of the possible behaviors.
| def test_expanduser_filename(mock_home_env, tmpdir): | ||
| DATA_DIR = tmpdir | ||
| INFO_LOG = os.path.join(DATA_DIR, "monkey_island.log") | ||
| LOG_LEVEL = "DEBUG" |
There was a problem hiding this comment.
Since this is testing the call to logger.info(), let's make this log level INFO. Or we could leave this as DEBUG and call logger.debug() instead.
We should also add another test where we set the log level to INFO, call logger.debug(), and verify that the file does not contain the line.
| @@ -17,11 +12,13 @@ def mock_home_env(monkeypatch, tmpdir): | |||
| monkeypatch.setenv("HOME", str(tmpdir)) | |||
There was a problem hiding this comment.
I don't think this fixture is needed anymore since we're passing data_dir directly to the logger configuration function..
DeepCode's analysis on #b13839 found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
Because `monkey_island.py` has the same name as the `monkey_island` module, pytest can't import monkey_island.py and run any tests against its code.
As the number of configuration items will increase in the future, return the config dict instead of individual config properties.
Partial fix for #1146
PR Checklist
Was the documentation framework updated to reflect the changes?Testing Checklist