Conversation
ghost
self-requested a review
as a 
ghost
changed the title
| pub source: std::io::Error, | ||
| } | ||
|
|
||
| pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<(), WriteFileError> { |
There was a problem hiding this comment.
What is the reasoning behind these thin wrappers around the std library functions?
There was a problem hiding this comment.
These add path, in order to avoid reporting errors like "OS error 2: file not found" without telling which path wasn't found.
And they do it in one place, so other error types can include these as transparent rather than each defining their own error with std::io::Error as a source, path as context, and error text.
|
|
||
| pub fn save_json_file<T: Serialize>(path: &Path, value: &T) -> Result<(), SaveJsonFileError> { | ||
| let content = serde_json::to_string_pretty(&value).context(SerializeSnafu { path })?; | ||
| crate::fs::write(path, content)?; |
There was a problem hiding this comment.
Same question - what is the reasoning behind having these thin wrappers?
There was a problem hiding this comment.
These also add path context, and report a single error type that differentiates between i/o errors and json parse errors.
| use serde::Deserialize; | ||
|
|
||
| #[derive(Deserialize)] | ||
| pub struct ConnectedNetworkModel { |
There was a problem hiding this comment.
Can you add a comment explaining what a "connected network" is?
There was a problem hiding this comment.
Would "remote network" be a better terminology here?
There was a problem hiding this comment.
Not necessarily. It could refer to another network that happens to be running locally, like another project.
There was a problem hiding this comment.
From the project's point of view, wouldn't that network be remote?
| use std::path::Path; | ||
| use time::OffsetDateTime; | ||
|
|
||
| pub struct PocketIcAdminInterface { |
There was a problem hiding this comment.
Is the reason you are creating PocketIcAdminInterface due to pocket_ic not accepting an http client in its initializer? It might be simpler to ask Martin to support taking an optional transport.
There was a problem hiding this comment.
No, it's because the access library in pocket_ic does a bunch of things under the hood that we don't want, such as downloading the pocket-ic binary in some conditions and panicking in others. It's more geared to running tests than a user-facing application.
| cmd.args(["--ttl", "2592000", "--log-levels", "error"]); | ||
|
|
||
| cmd.stdout(std::process::Stdio::inherit()); | ||
| cmd.stderr(std::process::Stdio::inherit()); |
There was a problem hiding this comment.
Is this supposed to use Stderr?
There was a problem hiding this comment.
Thanks for noticing, I didn't. I actually brought this over from https://github.com/dfinity/sdk/blob/master/src/dfx/src/actors/pocketic_proxy.rs#L259 or https://github.com/dfinity/sdk/blob/master/src/dfx/src/actors/pocketic.rs#L249. I think in this context, stdout is okay.
| @@ -0,0 +1,42 @@ | |||
| use std::path::{Path, PathBuf}; | |||
|
|
|||
| pub struct NetworkDirectoryStructure { | |||
There was a problem hiding this comment.
Is Structure an equivalent term to manifest?
There was a problem hiding this comment.
My intent with this struct is to provide a single, focused place to describe the structure of a network directory by returning known paths.
We could consider renaming it to something like NetworkLayout or NetworkFilesystemLayout.
Regarding the term "manifest", I generally understand that to mean a populated list of specific contents. For example, a Cargo.toml file is a manifest: a list of packages and dependencies. A list of the fields one might find in a Cargo.toml would be more of a schema. Similarly, a shipping or flight manifest is a list of passengers or cargo. If such a manifest hasn't been filled in yet, we might call it a template rather than a manifest.
While one could argue that a known set of paths constitutes "contents" and thus a kind of manifest, I think that’s a stretch. We're describing the structure or layout of a directory. The contents are what's inside the files, not the locations of the files themselves.
- work dir: .icp/ - network work dir root: .icp/networks - describe what a managed network is Also renamed the pocket-ic work dir from .pocketic to pocket-ic. No need for two "dot" files in path, and consistent with other pocket-ic naming.
1 participant
Adds a command,
icp network run, which runs the local network in the foreground until the user presses Ctrl-C or otherwise sends SIGINT.Adds a test that dfx can deploy to this network.
Everything is hardcoded and there are various rough edges. As discussed, this is intended to be a starting point for further iteration and collaboration.