1576 simplify server config json #1633
Conversation
Options removed match the defaults so there's no reason to keep them
…t tests for it Since server_config.json no longer needs to be writable, we can load defaults, then override package specific options and lastly override user specified options to form the final config for island
On windows it's not easy to pass server_config as a commandline parameter. It's easier to just create a file in install directory.
…configuration.md page, which explains how to setup and use server_configuration.json This change extracts server_config.json usage into a single page, which can then be referred to from any page that requires island configuration
d411983 to
3e32dbb
Compare
…d adding default parameter to IslandConfigOptions
…nfig_setup.py, where they are used
…evels `info` and `debug`, instead of saying "default Python log levels"
Codecov Report
@@ Coverage Diff @@
## develop #1633 +/- ##
===========================================
+ Coverage 44.75% 45.10% +0.35%
===========================================
Files 460 459 -1
Lines 13398 13400 +2
===========================================
+ Hits 5996 6044 +48
+ Misses 7402 7356 -46
Continue to review full report at Codecov.
|
…nspecified properties to get overridden by defaults
…n island's cwd All deployments can be configured via command line OR by modifying the server_config.json that comes with the deployment
…and add a UT for that
…ocs to corresponding deployment option setup pages
Agents' log level is not configurable at this time.
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
When would there ever be a KeyError? Shouldn't the .get() calls avoid this?
There was a problem hiding this comment.
Key error will happen when some config value is NOT provided in the server_config.json. We can't create IslandConfigOptions from the contents of the file, because unspecified options will get defaults. Then, how will we know if user provided a value that overrides package config OR was it just a default and we shouldn't override the package config with it?
There was a problem hiding this comment.
Ah, I see. "." is the separator, not the default value.
Why don't we change the order of the calls in update(). Call self.__dict__.update(), then call self._expand_config_paths(). This way, we'll be guaranteed that all paths exist.
|
|
||
| import monkey_island.cc.setup.config_setup # noqa: F401 | ||
| from monkey_island.cc.arg_parser import IslandCmdArgs | ||
| from monkey_island.cc.server_setup import _extract_config |
There was a problem hiding this comment.
Testing private/internal methods/functions is generally bad practice. Unit tests are for testing public interfaces.
Since _extract_config() is mostly just a wrapper around get_server_config(), let's just call that function instead. Then we can get rid of test_malformed_json() and simplify the call to create_server_config().
There was a problem hiding this comment.
It's important to test what happens if the user provides a malformed JSON and it mostly uses the same fixtures to do so. I've tried splitting this up, but I don't think it's making this any more readable or maintainable, it just makes it more granular. I'm leaving it as it is.
What does this PR do?
Fixes #1576
Add any further explanations here.
PR Checklist
Testing Checklist
Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:
If applicable, add screenshots or log transcripts of the feature working
Explain Changes
Are the commit messages enough? If not, elaborate.