Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create data directory on Island initialisation #1170
Create data directory on Island initialisation #1170
Changes from 22 commits
76d82ce
8246341
808e86d
9eedac4
805e5e6
d8927a5
af42c01
ff1e6bd
a1beee9
3201672
e7a26aa
8c575b9
8ce506a
409a3c5
2fb77fc
37b7815
dc129c0
4640a76
b4708fc
9705c73
23b0492
43d919a
70b9a9f
8afe937
5f88f6f
3098ac1
d33c5d6
f500ca8
7d1c5dd
0a7cf1d
2b257f0
d273e85
3a800d9
9337c68
057a579
5b7329b
17e994c
a89a62c
baee74b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line only makes sense if
island_args.server_config
, because otherwise you create the dir inserver_config_generator.create_default_server_config_file()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #1184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I still don't understand what's wrong with using
os.makedirs
here. The explanation of$HOME
might not be resolved doesn't make sense. It's the responsibility ofDEFAULT_DATA_DIR
to point to a resolvable path. And ifDEFAULT_DATA_DIR
can't be resolved, how will not making parent directories help us?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, on Linux, if the $HOME env variable doesn't exist, the value of
DEFAULT_DATA_DIR
will remain "$HOME/.monkey_island" (we're usingos.path.expandvars()
to resolve the path but in case nothing is resolved, it will simply return the same string, it won't throw any kind of error). Usingos.makedirs
will create a directory named "$HOME" and the ".monkey_island" directory under that. Creating a directory named "$HOME" is not the expected behaviour. If the $HOME variable can't be resolved, we expect the user to specify the path for the data directory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this expressed in the code and where? Users and developers are expected to get
os.mkdir
error and know thatos.mkdir
failed because it doesn't create parent folders, so it means that the parent folder ofdata_dir
is not present?What prevents the user from making the same mistake via cmd arguments?
Wouldn't it be better to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See baee74b7