liurenjie1024 commented on code in PR #489:
URL: https://github.com/apache/iceberg-rust/pull/489#discussion_r1697072080
##########
crates/test_utils/src/cmd.rs:
##########
@@ -28,14 +28,27 @@ pub fn run_command(mut cmd: Command, desc: impl ToString) {
}
}
-pub fn get_cmd_output(mut cmd: Command, desc: impl ToString) -> String {
+pub fn get_cmd_output_result(mut cmd: Command, desc: impl ToString) ->
Result<String, String> {
let desc = desc.to_string();
log::info!("Starting to {}, command: {:?}", &desc, cmd);
- let output = cmd.output().unwrap();
- if output.status.success() {
- log::info!("{} succeed!", desc);
- String::from_utf8(output.stdout).unwrap()
- } else {
- panic!("{} failed: {:?}", desc, output.status);
+ let result = cmd.output();
+ match result {
+ Ok(output) => {
+ if output.status.success() {
+ log::info!("{} succeed!", desc);
+ Ok(String::from_utf8(output.stdout).unwrap())
+ } else {
+ Err(format!("{} failed with rc: {:?}", desc, output.status))
+ }
+ }
+ Err(_err) => Err(format!("{} failed to execute", desc)),
Review Comment:
I don't think we should ignore this error here, we should at least display
`_err` in error message so that it would be easier to know what's happening.
##########
CONTRIBUTING.md:
##########
@@ -110,14 +110,16 @@ $ cargo version
cargo 1.69.0 (6e9a83356 2023-04-12)
```
-#### Install docker
+#### Install Docker or Podman
-Currently, iceberg-rust uses docker to set up environment for integration
tests.
+Currently, iceberg-rust uses Docker to set up environment for integration
tests. Podman is also supported.
-You can learn how to install docker from
[here](https://docs.docker.com/get-docker/).
+You can learn how to install Docker from
[here](https://docs.docker.com/get-docker/).
For macos users, you can install [OrbStack](https://orbstack.dev/) as a docker
alternative.
+For podman users, refer to [Using Podman instead of
Docker](https://rust.iceberg.apache.org/reference/podman.md)
Review Comment:
+1 with keeping user guide only. Contribution and release are developer
oriented, and it's safe to keep them in github so that they always stay in sync
with codes. I took a look at serveral crates and they all keep user doc only:
https://serde.rs/
https://tokio.rs/
##########
crates/catalog/rest/testdata/rest_catalog/docker-compose.yaml:
##########
@@ -15,6 +15,9 @@
# specific language governing permissions and limitations
# under the License.
+networks:
Review Comment:
Thanks for explaination, I didn't know link was deprecated, this sounds
reasonable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
