Output error or warning without panic.

dfinity · Sep 23, 2024 · 284f009 · 284f009
1 parent 1286e76
commit 284f009
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 14 deletions.
diff --git a/e2e/tests-dfx/update_settings.bash b/e2e/tests-dfx/update_settings.bash
@@ -105,6 +105,9 @@ teardown() {
   assert_command dfx canister status e2e_project_backend
   assert_contains "${ALICE_PRINCIPAL}"
 
+  assert_command dfx canister update-settings --remove-log-viewer="${BOB_PRINCIPAL}" e2e_project_backend
+  assert_contains "'${BOB_PRINCIPAL}' is not in the allowed list"
+
   assert_command dfx canister update-settings --add-log-viewer="${BOB_PRINCIPAL}" --remove-log-viewer="${ALICE_PRINCIPAL}" e2e_project_backend
   assert_command dfx canister status e2e_project_backend
   assert_contains "${BOB_PRINCIPAL}"
@@ -137,6 +140,9 @@ teardown() {
   assert_command dfx canister status e2e_project_backend
   assert_contains "Log visibility: controllers"
 
+  assert_command_fail dfx canister update-settings --remove-log-viewer="${BOB_PRINCIPAL}" e2e_project_backend
+  assert_contains "Removing reviewers is not allowed with 'public' or 'controllers' log visibility."
+
   # Test --all code path.
   assert_command dfx canister update-settings --add-log-viewer="${ALICE_PRINCIPAL}" --all
   assert_command dfx canister status e2e_project_backend

diff --git a/src/dfx/src/lib/canister_logs/log_visibility.rs b/src/dfx/src/lib/canister_logs/log_visibility.rs
@@ -1,6 +1,7 @@
 use crate::lib::environment::Environment;
 use crate::lib::error::DfxResult;
 use crate::util::clap::parsers::log_visibility_parser;
+use anyhow::anyhow;
 use candid::Principal;
 use clap::{ArgAction, Args};
 use dfx_core::cli::ask_for_consent;
@@ -47,7 +48,7 @@ impl LogVisibilityOpt {
         &self,
         env: &dyn Environment,
         current_status: Option<&StatusCallResult>,
-    ) -> Result<LogVisibility, String> {
+    ) -> DfxResult<LogVisibility> {
         let logger = env.get_logger();
 
         // For public and controllers.
@@ -81,11 +82,8 @@ impl LogVisibilityOpt {
         // Adding.
         if let Some(added) = self.add_log_viewer.as_ref() {
             if let Some(LogVisibility::Public) = current_visibility {
-                // TODO:
-                // Warning for taking away view rights for everyone.
-
                 let msg = "Current log is public to everyone. Adding log reviewers will make the log only visible to the reviewers.";
-                ask_for_consent(msg).map_err(|e| e.to_string())?;
+                ask_for_consent(msg)?;
             }
 
             for viewer in added {
@@ -101,9 +99,7 @@ impl LogVisibilityOpt {
             if let Some(visibility) = &current_visibility {
                 match visibility {
                     LogVisibility::Public | LogVisibility::Controllers => {
-                        slog::warn!(logger, "WARNING!");
-                        slog::warn!(logger, "Removing reviewers is not allowed with 'public' or 'controllers' log visibility");
-                        return Err("Removing reviewers is not allowed with 'public' or 'controllers' log visibility.".to_string());
+                        return Err(anyhow!("Removing reviewers is not allowed with 'public' or 'controllers' log visibility."));
                     }
                     _ => (),
                 }
@@ -113,7 +109,6 @@ impl LogVisibilityOpt {
                 if let Some(idx) = viewers.iter().position(|x| *x == principal) {
                     viewers.swap_remove(idx);
                 } else {
-                    slog::warn!(logger, "WARNING!");
                     slog::warn!(logger, "Principal '{}' is not in the allowed list.", viewer);
                 }
             }

diff --git a/src/dfx/src/lib/ic_attributes/mod.rs b/src/dfx/src/lib/ic_attributes/mod.rs
@@ -227,11 +227,9 @@ pub fn get_log_visibility(
     canister_name: Option<&str>,
 ) -> DfxResult<Option<LogVisibility>> {
     let log_visibility = match (log_visibility, config_interface, canister_name) {
-        (Some(log_visibility), _, _) => Some(
-            log_visibility
-                .to_log_visibility(env, current_settings)
-                .unwrap(),
-        ),
+        (Some(log_visibility), _, _) => {
+            Some(log_visibility.to_log_visibility(env, current_settings)?)
+        }
         (None, Some(config_interface), Some(canister_name)) => {
             config_interface.get_log_visibility(canister_name)?
         }