-
Notifications
You must be signed in to change notification settings - Fork 95
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
Parameterize the wallet file location #580
Parameterize the wallet file location #580
Conversation
Allows for setting custom wallet file locations and running multiple instances in the same environment.
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.
Approving with some nits, please fix them before merging.
@@ -54,6 +55,9 @@ public class SubzeroGui { | |||
// Almost always you want to talk to subzero on localhost | |||
@Parameter(names = "--hostname") public String hostname = "localhost"; | |||
|
|||
// Wallet file location | |||
@Parameter(names = "--wallet-file") public String walletFilePath = "/data/app/subzero/wallets/"; |
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.
nit: I think this should be called --wallet-dir
or similar since it's actually the path to the wallet 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.
nit: and similarly call the variable walletDirectory
or something like that, since it's neither a Path object (it's a String), nor names the wallet file.
* Will throw runtime exceptions if the path provided is invalid. | ||
*/ | ||
public WalletLoader(String walletFilePath) { | ||
directory = FileSystems.getDefault().getPath(walletFilePath); |
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.
super nit: this can call the other constructor instead: this(FileSystems ...)
, this way if the Path constructor gets more logic later we don't need to duplicate it here.
@Test | ||
public void walletLoaderBadFilePaths() throws Exception { | ||
assertThatThrownBy(() -> new WalletLoader("\0")) | ||
.isInstanceOf(IllegalArgumentException.class); | ||
} | ||
|
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.
super nit: should we also add the following test cases?
- path looks well-formed but doesn't exist
- path refers to a regular file rather than a 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.
i didn't include these because they get handled by the marker file check, but it doesn't hurt to have the tests explicitly in case something changes later.
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.
LGTM concept ack
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.
LGTM
Allows for setting custom wallet file locations and running multiple instances in the same environment.