From 7e62ea6f06a5c76734b1fa7e17d81b9d624f1506 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Mon, 21 Oct 2024 14:41:54 +0100
Subject: [PATCH 01/11] Verify locations on addition

---
 R/location.R                   | 39 +++++++++++++++++++++++++---------
 R/location_http.R              |  4 ++++
 tests/testthat/test-location.R |  3 +++
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/R/location.R b/R/location.R
index 1bdabf15..dd534228 100644
--- a/R/location.R
+++ b/R/location.R
@@ -75,26 +75,27 @@
 ##' @param args Arguments to the location driver. The arguments here
 ##'   will vary depending on the type used, see Details.
 ##'
+##' @param verify Logical, indicating if we should verify that the
+##'   location can be used before adding.
+##'
 ##' @inheritParams orderly_metadata
 ##'
 ##' @return Nothing
 ##' @export
-orderly_location_add <- function(name, type, args, root = NULL) {
+orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
-  assert_scalar_character(name, call = environment())
+  assert_scalar_character(name)
+  assert_scalar_logical(verify)
 
   if (name %in% location_reserved_name) {
     cli::cli_abort("Cannot add a location with reserved name '{name}'")
   }
 
   location_check_new_name(root, name, environment())
-  match_value(type, setdiff(location_types, location_reserved_name),
-              call = environment())
+  match_value(type, setdiff(location_types, location_reserved_name))
 
-  loc <- new_location_entry(name, type, args, call = environment())
   if (type == "path") {
     assert_scalar_character(args$path, name = "path")
-    root_open(args$path, require_orderly = FALSE)
   } else if (type == "http") {
     assert_scalar_character(args$url, name = "url")
   } else if (type == "packit") {
@@ -105,6 +106,19 @@ orderly_location_add <- function(name, type, args, root = NULL) {
     if (!is.null(args$token) && !is.null(args$save_token)) {
       cli::cli_abort("Cannot specify both 'token' and 'save_token'")
     }
+    if (is.null(args$save_token)) {
+      args$save_token <- is.null(args$token)
+    }
+  }
+  loc <- new_location_entry(name, type, args, call = environment())
+
+  if (verify) {
+    cli::cli_alert_info("Testing location")
+    driver <- location_driver_create(type, args)
+    if (!is.null(driver$authorise)) {
+      driver$authorise()
+    }
+    cli::cli_alert_success("Location configured successfully")
   }
 
   config <- root$config
@@ -483,6 +497,11 @@ location_driver <- function(location_name, root) {
   i <- match(location_name, root$config$location$name)
   type <- root$config$location$type[[i]]
   args <- root$config$location$args[[i]]
+  location_driver_create(type, args)
+}
+
+
+location_driver_create <- function(type, args) {
   location <- switch(type,
                      path = orderly_location_path$new,
                      http = orderly_location_http$new,
@@ -794,8 +813,8 @@ location_build_push_plan <- function(packet_id, location_name, root) {
 ## This validation probably will need generalising in future as we add
 ## new types. The trick is going to be making sure that we can support
 ## different location types in different target languages effectively.
-new_location_entry <- function(name, type, args, call = NULL) {
-  match_value(type, location_types, call = call)
+new_location_entry <- function(name, type, args, call = parent.frame()) {
+  match_value(type, location_types)
   required <- NULL
   if (type == "path") {
     required <- "path"
@@ -807,8 +826,8 @@ new_location_entry <- function(name, type, args, call = NULL) {
     required <- "driver"
   }
   if (length(args) > 0) {
-    assert_is(args, "list", call = call)
-    assert_named(args, call = call)
+    assert_is(args, "list")
+    assert_named(args)
   }
   msg <- setdiff(required, names(args))
   if (length(msg) > 0) {
diff --git a/R/location_http.R b/R/location_http.R
index 60ce6ed4..eeddc4c8 100644
--- a/R/location_http.R
+++ b/R/location_http.R
@@ -8,6 +8,10 @@ orderly_location_http <- R6::R6Class(
       self$client <- outpack_http_client$new(url, authorise)
     },
 
+    authorise = function() {
+      self$client$authorise()
+    },
+
     list = function() {
       dat <- self$client$request("/metadata/list")$data
       data_frame(
diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index 572c6258..06915c97 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -846,6 +846,9 @@ test_that("can add a packit location without a token", {
   orderly_location_add("other", "packit",
                        list(url = "https://example.com"),
                        root = root)
+  expect_equal(
+    orderly_config(root)$location$args[[2]],
+    url = "https://example.com", save_token = TRUE)
   expect_equal(orderly_location_list(root = root), c("local", "other"))
 
   mock_driver <- mockery::mock()

From b7bb3770c2aed768a6e9efd5693fe8899584a860 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Mon, 21 Oct 2024 15:39:58 +0100
Subject: [PATCH 02/11] Control verbosity, fix tests

---
 R/location.R                          | 53 +++++++++++---------
 R/location_packit.R                   | 13 +++--
 R/outpack_http_client.R               |  2 +-
 man/orderly_location_add.Rd           | 22 +++++++--
 tests/testthat/test-location-packit.R |  4 ++
 tests/testthat/test-location.R        | 70 +++++++++++++++++----------
 6 files changed, 107 insertions(+), 57 deletions(-)

diff --git a/R/location.R b/R/location.R
index dd534228..ba90a3b5 100644
--- a/R/location.R
+++ b/R/location.R
@@ -78,14 +78,20 @@
 ##' @param verify Logical, indicating if we should verify that the
 ##'   location can be used before adding.
 ##'
+##' @param quiet Logical, indicating if we should print information
+##'   while configuring and creating locations.  If not given, we use
+##'   the option of `orderly.quiet`, defaulting to `TRUE`.
+##'
 ##' @inheritParams orderly_metadata
 ##'
 ##' @return Nothing
 ##' @export
-orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
+orderly_location_add <- function(name, type, args, verify = TRUE, quiet = NULL,
+                                 root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
   assert_scalar_character(name)
   assert_scalar_logical(verify)
+  quiet <- orderly_quiet(quiet)
 
   if (name %in% location_reserved_name) {
     cli::cli_abort("Cannot add a location with reserved name '{name}'")
@@ -113,12 +119,16 @@ orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
   loc <- new_location_entry(name, type, args, call = environment())
 
   if (verify) {
-    cli::cli_alert_info("Testing location")
+    if (!quiet) {
+      cli::cli_alert_info("Testing location")
+    }
     driver <- location_driver_create(type, args)
     if (!is.null(driver$authorise)) {
       driver$authorise()
     }
-    cli::cli_alert_success("Location configured successfully")
+    if (!quiet) {
+      cli::cli_alert_success("Location configured successfully")
+    }
   }
 
   config <- root$config
@@ -136,9 +146,11 @@ orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
 ##'   be unreliable.
 ##'
 ##' @export
-orderly_location_add_path <- function(name, path, root = NULL) {
+orderly_location_add_path <- function(name, path, verify = TRUE, quiet = NULL,
+                                      root = NULL) {
   args <- list(path = path)
-  orderly_location_add(name, "path", args, root = root)
+  orderly_location_add(name, "path", args, verify = verify, quiet = quiet,
+                       root = root)
 }
 
 
@@ -148,9 +160,11 @@ orderly_location_add_path <- function(name, path, root = NULL) {
 ##'   example `http://example.com:8080`
 ##'
 ##' @export
-orderly_location_add_http <- function(name, url, root = NULL) {
+orderly_location_add_http <- function(name, url, verify = TRUE, quiet = NULL,
+                                      root = NULL) {
   args <- list(url = url)
-  orderly_location_add(name, "http", args, root = root)
+  orderly_location_add(name, "http", args, verify = verify, quiet = quiet,
+                       root = root)
 }
 
 
@@ -167,9 +181,12 @@ orderly_location_add_http <- function(name, url, root = NULL) {
 ##'
 ##' @export
 orderly_location_add_packit <- function(name, url, token = NULL,
-                                        save_token = NULL, root = NULL) {
+                                        save_token = NULL,
+                                        verify = TRUE, quiet = NULL,
+                                        root = NULL) {
   args <- list(url = url, token = token, save_token = save_token)
-  orderly_location_add(name, "packit", args, root = root)
+  orderly_location_add(name, "packit", args, verify = verify, quiet = quiet,
+                       root = root)
 }
 
 
@@ -815,26 +832,14 @@ location_build_push_plan <- function(packet_id, location_name, root) {
 ## different location types in different target languages effectively.
 new_location_entry <- function(name, type, args, call = parent.frame()) {
   match_value(type, location_types)
-  required <- NULL
-  if (type == "path") {
-    required <- "path"
-  } else if (type == "http") {
-    required <- "url"
-  } else if (type == "packit") {
-    required <- "url"
-  } else if (type == "custom") {
-    required <- "driver"
-  }
   if (length(args) > 0) {
     assert_is(args, "list")
     assert_named(args)
   }
-  msg <- setdiff(required, names(args))
-  if (length(msg) > 0) {
-    cli::cli_abort("Field{?s} missing from args: {squote(msg)}")
-  }
-
   if (type == "custom") {
+    if (is.null(args$driver)) {
+      cli::cli_abort("Field missing from args: 'driver'")
+    }
     check_symbol_from_str(args$driver, "args$driver")
   }
 
diff --git a/R/location_packit.R b/R/location_packit.R
index 5f32dcd0..0dad14d6 100644
--- a/R/location_packit.R
+++ b/R/location_packit.R
@@ -42,17 +42,20 @@ do_oauth_device_flow <- function(base_url, cache_disk) {
 # It also means the user cannot easily use two different identities on the same
 # server from within the same session.
 auth_cache <- new.env(parent = emptyenv())
-packit_authorisation <- function(base_url, token, save_token) {
+packit_authorisation <- function(base_url, token, save_token, quiet = NULL) {
   # If a non-Github token is provided, we assume it is a native Packit token
   # and use that directly.
   if (!is.null(token) && !grepl("^gh._", token)) {
     return(list("Authorization" = paste("Bearer", token)))
   }
 
+  quiet <- orderly_quiet(quiet)
   key <- rlang::hash(list(base_url = base_url, token = token))
 
   if (is.null(auth_cache[[key]])) {
-    cli::cli_alert_info("Logging in to {base_url}")
+    if (!quiet) {
+      cli::cli_alert_info("Logging in to {base_url}")
+    }
     if (is.null(token)) {
       token <- do_oauth_device_flow(base_url, cache_disk = save_token)
     }
@@ -62,7 +65,9 @@ packit_authorisation <- function(base_url, token, save_token) {
       login_url,
       function(r) http_body_json(r, list(token = scalar(token))))
 
-    cli::cli_alert_success("Logged in successfully")
+    if (!quiet) {
+      cli::cli_alert_success("Logged in successfully")
+    }
 
     auth_cache[[key]] <- list("Authorization" = paste("Bearer", res$token))
   }
@@ -89,5 +94,5 @@ orderly_location_packit <- function(url, token = NULL, save_token = TRUE) {
 
   orderly_location_http$new(
     paste0(url, "packit/api/outpack"),
-    function() packit_authorisation(url, token, save_token))
+    function(quiet = NULL) packit_authorisation(url, token, save_token, quiet))
 }
diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R
index 880a8ba0..bb9e0cf4 100644
--- a/R/outpack_http_client.R
+++ b/R/outpack_http_client.R
@@ -8,7 +8,7 @@ outpack_http_client <- R6::R6Class(
     initialize = function(url, authorise = NULL) {
       self$url <- sub("/$", "", url)
       if (is.null(authorise)) {
-        self$authorise <- function() NULL
+        self$authorise <- function(...) NULL
       } else {
         self$authorise <- authorise
       }
diff --git a/man/orderly_location_add.Rd b/man/orderly_location_add.Rd
index 82be47b9..8ac26862 100644
--- a/man/orderly_location_add.Rd
+++ b/man/orderly_location_add.Rd
@@ -7,17 +7,26 @@
 \alias{orderly_location_add_packit}
 \title{Add a new location}
 \usage{
-orderly_location_add(name, type, args, root = NULL)
+orderly_location_add(
+  name,
+  type,
+  args,
+  verify = TRUE,
+  quiet = NULL,
+  root = NULL
+)
 
-orderly_location_add_path(name, path, root = NULL)
+orderly_location_add_path(name, path, verify = TRUE, quiet = NULL, root = NULL)
 
-orderly_location_add_http(name, url, root = NULL)
+orderly_location_add_http(name, url, verify = TRUE, quiet = NULL, root = NULL)
 
 orderly_location_add_packit(
   name,
   url,
   token = NULL,
   save_token = NULL,
+  verify = TRUE,
+  quiet = NULL,
   root = NULL
 )
 }
@@ -33,6 +42,13 @@ API).}
 \item{args}{Arguments to the location driver. The arguments here
 will vary depending on the type used, see Details.}
 
+\item{verify}{Logical, indicating if we should verify that the
+location can be used before adding.}
+
+\item{quiet}{Logical, indicating if we should print information
+while configuring and creating locations.  If not given, we use
+the option of \code{orderly.quiet}, defaulting to \code{TRUE}.}
+
 \item{root}{The path to the root directory, or \code{NULL} (the
 default) to search for one from the current working
 directory. This function does not require that the directory is
diff --git a/tests/testthat/test-location-packit.R b/tests/testthat/test-location-packit.R
index 15f2cb23..b8f152a3 100644
--- a/tests/testthat/test-location-packit.R
+++ b/tests/testthat/test-location-packit.R
@@ -1,4 +1,5 @@
 test_that("can authenticate with existing GitHub token", {
+  withr::local_options(orderly.quiet = FALSE)
   clear_auth_cache()
   withr::defer(clear_auth_cache())
 
@@ -43,6 +44,7 @@ test_that("can authenticate with existing Packit token", {
 
 
 test_that("can authenticate using device flow", {
+  withr::local_options(orderly.quiet = FALSE)
   clear_auth_cache()
   withr::defer(clear_auth_cache())
 
@@ -70,7 +72,9 @@ test_that("can authenticate using device flow", {
   expect_equal(args[[1]]$body$type, "json")
 })
 
+
 test_that("location_packit uses authentication", {
+  withr::local_options(orderly.quiet = FALSE)
   clear_auth_cache()
   withr::defer(clear_auth_cache())
 
diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index 06915c97..cd058f88 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -779,7 +779,7 @@ test_that("validate arguments to path locations", {
   expect_error(
     orderly_location_add("other", "path", list(root = "mypath"),
                          root = root),
-    "Field missing from args: 'path'")
+    "'path' must be a scalar")
   expect_equal(orderly_location_list(root = root), "local")
 })
 
@@ -789,7 +789,7 @@ test_that("validate arguments to http locations", {
   expect_error(
     orderly_location_add("other", "http", list(server = "example.com"),
                          root = root),
-    "Field missing from args: 'url'")
+    "'url' must be a scalar")
   expect_equal(orderly_location_list(root = root), "local")
 })
 
@@ -799,16 +799,22 @@ test_that("validate arguments to packit locations", {
   expect_error(
     orderly_location_add("other", "packit", list(server = "example.com"),
                          root = root),
-    "Field missing from args: 'url'")
+    "'url' must be a scalar")
 
   expect_error(
-    orderly_location_add_packit("other", url = "example.com", token = 123,
+    orderly_location_add_packit("other",
+                                url = "example.com",
+                                token = 123,
+                                verify = FALSE,
                                 root = root),
     "Expected 'token' to be character", fixed = TRUE)
 
   expect_error(
-    orderly_location_add_packit("other", "packit", url = "example.com",
-                                save_token = "value", root = root),
+    orderly_location_add_packit("other",
+                                url = "example.com",
+                                save_token = "value",
+                                verify = FALSE,
+                                root = root),
     "Expected 'save_token' to be logical", fixed = TRUE)
 
   expect_error(
@@ -816,6 +822,7 @@ test_that("validate arguments to packit locations", {
                                 url = "example.com",
                                 token = "xx",
                                 save_token = TRUE,
+                                verify = FALSE,
                                 root = root),
     "Cannot specify both 'token' and 'save_token'", fixed = TRUE)
 
@@ -825,41 +832,48 @@ test_that("validate arguments to packit locations", {
 
 test_that("can add a packit location", {
   root <- create_temporary_root()
-  orderly_location_add("other", "packit",
-                       list(url = "https://example.com", token = "abc123"),
-                       root = root)
+  orderly_location_add_packit("other",
+                              url = "https://example.com",
+                              token = "abc123",
+                              verify = FALSE,
+                              root = root)
   expect_equal(orderly_location_list(root = root), c("local", "other"))
 
   mock_driver <- mockery::mock()
-  mockery::stub(location_driver, "orderly_location_packit", mock_driver)
+  mockery::stub(location_driver, "location_driver_create", mock_driver)
 
   dr <- location_driver("other", root)
 
   mockery::expect_called(mock_driver, 1)
   expect_equal(
     mockery::mock_args(mock_driver)[[1]],
-    list(url = "https://example.com", token = "abc123"))
+    list("packit",
+         list(url = "https://example.com",
+              token = "abc123",
+              save_token = FALSE)))
 })
 
 test_that("can add a packit location without a token", {
   root <- create_temporary_root()
-  orderly_location_add("other", "packit",
-                       list(url = "https://example.com"),
-                       root = root)
+  orderly_location_add_packit("other",
+                              url = "https://example.com",
+                              verify = FALSE,
+                              root = root)
   expect_equal(
     orderly_config(root)$location$args[[2]],
-    url = "https://example.com", save_token = TRUE)
+    list(url = "https://example.com", token = NULL, save_token = TRUE))
   expect_equal(orderly_location_list(root = root), c("local", "other"))
 
   mock_driver <- mockery::mock()
-  mockery::stub(location_driver, "orderly_location_packit", mock_driver)
+  mockery::stub(location_driver, "location_driver_create", mock_driver)
 
   dr <- location_driver("other", root)
 
   mockery::expect_called(mock_driver, 1)
   expect_equal(
     mockery::mock_args(mock_driver)[[1]],
-    list(url = "https://example.com"))
+    list("packit",
+         list(url = "https://example.com", token = NULL, save_token = TRUE)))
 })
 
 test_that("cope with trailing slash in url if needed", {
@@ -920,20 +934,20 @@ test_that("can add a custom outpack location", {
   skip_if_not_installed("mockery")
   root <- create_temporary_root()
   args <- list(driver = "foo::bar", a = 1, b = 2)
-  orderly_location_add("a", "custom", args = args, root = root)
+  orderly_location_add("a", "custom", args = args, verify = FALSE, root = root)
 
   loc <- as.list(root$config$location[2, ])
   expect_equal(loc$name, "a")
   expect_equal(loc$type, "custom")
   expect_equal(loc$args[[1]], list(driver = "foo::bar", a = 1, b = 2))
 
-  mock_orderly_location_custom <- mockery::mock("value")
-  mockery::stub(location_driver, "orderly_location_custom",
-                mock_orderly_location_custom)
+  mock_orderly_location_driver_create <- mockery::mock("value")
+  mockery::stub(location_driver, "location_driver_create",
+                mock_orderly_location_driver_create)
   expect_equal(location_driver(loc$name, root), "value")
-  mockery::expect_called(mock_orderly_location_custom, 1)
-  expect_equal(mockery::mock_args(mock_orderly_location_custom)[[1]],
-               list(driver = "foo::bar", a = 1, b = 2))
+  mockery::expect_called(mock_orderly_location_driver_create, 1)
+  expect_equal(mockery::mock_args(mock_orderly_location_driver_create)[[1]],
+               list("custom", list(driver = "foo::bar", a = 1, b = 2)))
 })
 
 
@@ -1133,7 +1147,13 @@ test_that("be chatty when pulling packets", {
   withr::local_options(orderly.quiet = FALSE)
   here <- create_temporary_root()
   there <- create_temporary_root()
-  orderly_location_add_path("server", path = there$path, root = here)
+  res <- evaluate_promise(
+    orderly_location_add_path("server", path = there$path, root = here))
+  expect_length(res$messages, 2)
+  expect_match(res$messages[[1]],
+               "Testing location")
+  expect_match(res$messages[[2]],
+               "Location configured successfully")
 
   res <- evaluate_promise(orderly_location_pull_metadata(root = here))
   expect_length(res$messages, 2)

From 36472bb0ac0503e3f62484c2e364992eb4c483ac Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Mon, 21 Oct 2024 15:46:19 +0100
Subject: [PATCH 03/11] Add test of verification not adding location

---
 tests/testthat/test-location.R | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index cd058f88..609c66cd 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -1183,3 +1183,18 @@ test_that("be chatty when pulling packets", {
   expect_match(res$messages[[2]],
                "Found 3 packets at 'server', of which 1 is new")
 })
+
+
+test_that("verify location on addition", {
+  root <- create_temporary_root()
+  path <- tempfile()
+
+  expect_error(
+    orderly_location_add_path("upstream", path = path, root = root))
+  expect_equal(orderly_location_list(root = root), "local")
+
+  expect_no_error(
+    orderly_location_add_path("upstream", path = path, verify = FALSE,
+                              root = root))
+  expect_equal(orderly_location_list(root = root), c("local", "upstream"))
+})

From 61fdab76679692d28d3cdcad5ad3b2eef18a4557 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Mon, 21 Oct 2024 16:11:39 +0100
Subject: [PATCH 04/11] Expand testing

---
 tests/testthat/test-location.R |  8 ++++++++
 tests/testthat/test-util.R     | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index 609c66cd..7f249294 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -951,6 +951,14 @@ test_that("can add a custom outpack location", {
 })
 
 
+test_that("custom drivers require a 'driver' argument", {
+  root <- create_temporary_root()
+  expect_error(
+    orderly_location_add("a", "custom", args = list(), root = root)
+    "Field missing from args: 'driver'")
+})
+
+
 test_that("can pull packets as a result of a query", {
   root <- list()
   for (name in c("src", "dst")) {
diff --git a/tests/testthat/test-util.R b/tests/testthat/test-util.R
index 74ce7a0e..69039359 100644
--- a/tests/testthat/test-util.R
+++ b/tests/testthat/test-util.R
@@ -589,3 +589,18 @@ describe("copy_files", {
                  "have different lengths")
   })
 })
+
+
+test_that("can sensibly control quietness", {
+  withr::with_options(list(orderly.quiet = NULL), {
+    expect_false(orderly_quiet(NULL))
+    expect_false(orderly_quiet(FALSE))
+    expect_true(orderly_quiet(TRUE))
+  })
+
+  withr::with_options(list(orderly.quiet = TRUE), {
+    expect_true(orderly_quiet(NULL))
+    expect_false(orderly_quiet(FALSE))
+    expect_true(orderly_quiet(TRUE))
+  })
+})

From 6acc8805039a701a9f1cc2bc5b85415947a9e0c1 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 08:52:14 +0100
Subject: [PATCH 05/11] Fix test file

---
 tests/testthat/test-location.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index 7f249294..ffc38624 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -954,7 +954,7 @@ test_that("can add a custom outpack location", {
 test_that("custom drivers require a 'driver' argument", {
   root <- create_temporary_root()
   expect_error(
-    orderly_location_add("a", "custom", args = list(), root = root)
+    orderly_location_add("a", "custom", args = list(), root = root),
     "Field missing from args: 'driver'")
 })
 

From b5d3bd4f2c26b8d143a56298a83fd78ace845488 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 15:26:15 +0100
Subject: [PATCH 06/11] Use new quiet idea

---
 R/location.R                | 34 +++++++++-------------------------
 R/location_packit.R         | 13 ++++---------
 man/orderly_location_add.Rd | 18 +++---------------
 3 files changed, 16 insertions(+), 49 deletions(-)

diff --git a/R/location.R b/R/location.R
index ba90a3b5..6479a48f 100644
--- a/R/location.R
+++ b/R/location.R
@@ -78,20 +78,14 @@
 ##' @param verify Logical, indicating if we should verify that the
 ##'   location can be used before adding.
 ##'
-##' @param quiet Logical, indicating if we should print information
-##'   while configuring and creating locations.  If not given, we use
-##'   the option of `orderly.quiet`, defaulting to `TRUE`.
-##'
 ##' @inheritParams orderly_metadata
 ##'
 ##' @return Nothing
 ##' @export
-orderly_location_add <- function(name, type, args, verify = TRUE, quiet = NULL,
-                                 root = NULL) {
+orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
   root <- root_open(root, require_orderly = FALSE)
   assert_scalar_character(name)
   assert_scalar_logical(verify)
-  quiet <- orderly_quiet(quiet)
 
   if (name %in% location_reserved_name) {
     cli::cli_abort("Cannot add a location with reserved name '{name}'")
@@ -119,16 +113,12 @@ orderly_location_add <- function(name, type, args, verify = TRUE, quiet = NULL,
   loc <- new_location_entry(name, type, args, call = environment())
 
   if (verify) {
-    if (!quiet) {
-      cli::cli_alert_info("Testing location")
-    }
+    cli_alert_info("Testing location")
     driver <- location_driver_create(type, args)
     if (!is.null(driver$authorise)) {
       driver$authorise()
     }
-    if (!quiet) {
-      cli::cli_alert_success("Location configured successfully")
-    }
+    cli_alert_success("Location configured successfully")
   }
 
   config <- root$config
@@ -146,11 +136,9 @@ orderly_location_add <- function(name, type, args, verify = TRUE, quiet = NULL,
 ##'   be unreliable.
 ##'
 ##' @export
-orderly_location_add_path <- function(name, path, verify = TRUE, quiet = NULL,
-                                      root = NULL) {
+orderly_location_add_path <- function(name, path, verify = TRUE, root = NULL) {
   args <- list(path = path)
-  orderly_location_add(name, "path", args, verify = verify, quiet = quiet,
-                       root = root)
+  orderly_location_add(name, "path", args, verify = verify, root = root)
 }
 
 
@@ -160,11 +148,9 @@ orderly_location_add_path <- function(name, path, verify = TRUE, quiet = NULL,
 ##'   example `http://example.com:8080`
 ##'
 ##' @export
-orderly_location_add_http <- function(name, url, verify = TRUE, quiet = NULL,
-                                      root = NULL) {
+orderly_location_add_http <- function(name, url, verify = TRUE, root = NULL) {
   args <- list(url = url)
-  orderly_location_add(name, "http", args, verify = verify, quiet = quiet,
-                       root = root)
+  orderly_location_add(name, "http", args, verify = verify, root = root)
 }
 
 
@@ -182,11 +168,9 @@ orderly_location_add_http <- function(name, url, verify = TRUE, quiet = NULL,
 ##' @export
 orderly_location_add_packit <- function(name, url, token = NULL,
                                         save_token = NULL,
-                                        verify = TRUE, quiet = NULL,
-                                        root = NULL) {
+                                        verify = TRUE, root = NULL) {
   args <- list(url = url, token = token, save_token = save_token)
-  orderly_location_add(name, "packit", args, verify = verify, quiet = quiet,
-                       root = root)
+  orderly_location_add(name, "packit", args, verify = verify, root = root)
 }
 
 
diff --git a/R/location_packit.R b/R/location_packit.R
index 0dad14d6..16f4dc7d 100644
--- a/R/location_packit.R
+++ b/R/location_packit.R
@@ -42,20 +42,17 @@ do_oauth_device_flow <- function(base_url, cache_disk) {
 # It also means the user cannot easily use two different identities on the same
 # server from within the same session.
 auth_cache <- new.env(parent = emptyenv())
-packit_authorisation <- function(base_url, token, save_token, quiet = NULL) {
+packit_authorisation <- function(base_url, token, save_token) {
   # If a non-Github token is provided, we assume it is a native Packit token
   # and use that directly.
   if (!is.null(token) && !grepl("^gh._", token)) {
     return(list("Authorization" = paste("Bearer", token)))
   }
 
-  quiet <- orderly_quiet(quiet)
   key <- rlang::hash(list(base_url = base_url, token = token))
 
   if (is.null(auth_cache[[key]])) {
-    if (!quiet) {
-      cli::cli_alert_info("Logging in to {base_url}")
-    }
+    cli_alert_info("Logging in to {base_url}")
     if (is.null(token)) {
       token <- do_oauth_device_flow(base_url, cache_disk = save_token)
     }
@@ -65,9 +62,7 @@ packit_authorisation <- function(base_url, token, save_token, quiet = NULL) {
       login_url,
       function(r) http_body_json(r, list(token = scalar(token))))
 
-    if (!quiet) {
-      cli::cli_alert_success("Logged in successfully")
-    }
+    cli_alert_success("Logged in successfully")
 
     auth_cache[[key]] <- list("Authorization" = paste("Bearer", res$token))
   }
@@ -94,5 +89,5 @@ orderly_location_packit <- function(url, token = NULL, save_token = TRUE) {
 
   orderly_location_http$new(
     paste0(url, "packit/api/outpack"),
-    function(quiet = NULL) packit_authorisation(url, token, save_token, quiet))
+    function() packit_authorisation(url, token, save_token))
 }
diff --git a/man/orderly_location_add.Rd b/man/orderly_location_add.Rd
index 8ac26862..609aa3f1 100644
--- a/man/orderly_location_add.Rd
+++ b/man/orderly_location_add.Rd
@@ -7,18 +7,11 @@
 \alias{orderly_location_add_packit}
 \title{Add a new location}
 \usage{
-orderly_location_add(
-  name,
-  type,
-  args,
-  verify = TRUE,
-  quiet = NULL,
-  root = NULL
-)
+orderly_location_add(name, type, args, verify = TRUE, root = NULL)
 
-orderly_location_add_path(name, path, verify = TRUE, quiet = NULL, root = NULL)
+orderly_location_add_path(name, path, verify = TRUE, root = NULL)
 
-orderly_location_add_http(name, url, verify = TRUE, quiet = NULL, root = NULL)
+orderly_location_add_http(name, url, verify = TRUE, root = NULL)
 
 orderly_location_add_packit(
   name,
@@ -26,7 +19,6 @@ orderly_location_add_packit(
   token = NULL,
   save_token = NULL,
   verify = TRUE,
-  quiet = NULL,
   root = NULL
 )
 }
@@ -45,10 +37,6 @@ will vary depending on the type used, see Details.}
 \item{verify}{Logical, indicating if we should verify that the
 location can be used before adding.}
 
-\item{quiet}{Logical, indicating if we should print information
-while configuring and creating locations.  If not given, we use
-the option of \code{orderly.quiet}, defaulting to \code{TRUE}.}
-
 \item{root}{The path to the root directory, or \code{NULL} (the
 default) to search for one from the current working
 directory. This function does not require that the directory is

From f461bfd28eb7104cafade91c5e64a691c5ebef8b Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 15:33:31 +0100
Subject: [PATCH 07/11] Drop extra test from previous version

---
 tests/testthat/test-util.R | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/tests/testthat/test-util.R b/tests/testthat/test-util.R
index 69039359..74ce7a0e 100644
--- a/tests/testthat/test-util.R
+++ b/tests/testthat/test-util.R
@@ -589,18 +589,3 @@ describe("copy_files", {
                  "have different lengths")
   })
 })
-
-
-test_that("can sensibly control quietness", {
-  withr::with_options(list(orderly.quiet = NULL), {
-    expect_false(orderly_quiet(NULL))
-    expect_false(orderly_quiet(FALSE))
-    expect_true(orderly_quiet(TRUE))
-  })
-
-  withr::with_options(list(orderly.quiet = TRUE), {
-    expect_true(orderly_quiet(NULL))
-    expect_false(orderly_quiet(FALSE))
-    expect_true(orderly_quiet(TRUE))
-  })
-})

From 2cce6f4cfccd6858b351e9c87f2aae801a697e1c Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 21:05:20 +0100
Subject: [PATCH 08/11] Bump version

---
 DESCRIPTION | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/DESCRIPTION b/DESCRIPTION
index b42ffab2..fe24b08b 100644
--- a/DESCRIPTION
+++ b/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: orderly2
 Title: Orderly Next Generation
-Version: 1.99.48
+Version: 1.99.49
 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"),
                     email = "rich.fitzjohn@gmail.com"),
              person("Robert", "Ashton", role = "aut"),

From 825af703bedf38bf26646827979bfc336741fdc8 Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 21:15:19 +0100
Subject: [PATCH 09/11] More general verification approach

---
 R/location.R      |  5 +----
 R/location_http.R | 12 ++++++++++++
 R/location_path.R |  4 ++++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/R/location.R b/R/location.R
index 6479a48f..fafc3d3c 100644
--- a/R/location.R
+++ b/R/location.R
@@ -114,10 +114,7 @@ orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
 
   if (verify) {
     cli_alert_info("Testing location")
-    driver <- location_driver_create(type, args)
-    if (!is.null(driver$authorise)) {
-      driver$authorise()
-    }
+    location_driver_create(type, args)$verify()
     cli_alert_success("Location configured successfully")
   }
 
diff --git a/R/location_http.R b/R/location_http.R
index eeddc4c8..2bd18a77 100644
--- a/R/location_http.R
+++ b/R/location_http.R
@@ -8,6 +8,18 @@ orderly_location_http <- R6::R6Class(
       self$client <- outpack_http_client$new(url, authorise)
     },
 
+    verify = function() {
+      ## This should never end up triggering the assertion here as
+      ## http_client_handle_error() in the client will convert
+      ## unsuccessful requests into an error already, but this should
+      ## serve as a reasonable backstop.
+      ##
+      ## The act of making the request will force validation, which is
+      ## the most likely source of errors (along with getting the URL
+      ## wrong).
+      stopifnot(identical(self$client$request("/")$status, "success"))
+    },
+
     authorise = function() {
       self$client$authorise()
     },
diff --git a/R/location_path.R b/R/location_path.R
index 3cb7ea30..25177227 100644
--- a/R/location_path.R
+++ b/R/location_path.R
@@ -10,6 +10,10 @@ orderly_location_path <- R6::R6Class(
       private$root <- root_open(path, require_orderly = FALSE)
     },
 
+    verify = function() {
+      ## No implementation needed as we verify the root in initialize()
+    },
+
     list = function() {
       private$root$index$location(local)[c("packet", "time", "hash")]
     },

From e38e4b2f039cb0c67ccb9ceaf61c34ad0802f2ec Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Tue, 22 Oct 2024 21:33:18 +0100
Subject: [PATCH 10/11] Tidy up the diff

---
 R/outpack_http_client.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R
index bb9e0cf4..880a8ba0 100644
--- a/R/outpack_http_client.R
+++ b/R/outpack_http_client.R
@@ -8,7 +8,7 @@ outpack_http_client <- R6::R6Class(
     initialize = function(url, authorise = NULL) {
       self$url <- sub("/$", "", url)
       if (is.null(authorise)) {
-        self$authorise <- function(...) NULL
+        self$authorise <- function() NULL
       } else {
         self$authorise <- authorise
       }

From a3a8da8d60628eb589d5d89a7632e0daac9010af Mon Sep 17 00:00:00 2001
From: Rich FitzJohn <r.fitzjohn@imperial.ac.uk>
Date: Wed, 23 Oct 2024 08:40:53 +0100
Subject: [PATCH 11/11] A bit more chat

---
 R/location.R                   | 1 +
 tests/testthat/test-location.R | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/R/location.R b/R/location.R
index fafc3d3c..9ca308ca 100644
--- a/R/location.R
+++ b/R/location.R
@@ -122,6 +122,7 @@ orderly_location_add <- function(name, type, args, verify = TRUE, root = NULL) {
   config$location <- rbind(config$location, loc)
   rownames(config$location) <- NULL
   config_update(config, root)
+  cli_alert_success("Added location '{name}' ({type})")
   invisible()
 }
 
diff --git a/tests/testthat/test-location.R b/tests/testthat/test-location.R
index ffc38624..b0f2baa4 100644
--- a/tests/testthat/test-location.R
+++ b/tests/testthat/test-location.R
@@ -1157,11 +1157,13 @@ test_that("be chatty when pulling packets", {
   there <- create_temporary_root()
   res <- evaluate_promise(
     orderly_location_add_path("server", path = there$path, root = here))
-  expect_length(res$messages, 2)
+  expect_length(res$messages, 3)
   expect_match(res$messages[[1]],
                "Testing location")
   expect_match(res$messages[[2]],
                "Location configured successfully")
+  expect_match(res$messages[[3]],
+               "Added location 'server' (path)", fixed = TRUE)
 
   res <- evaluate_promise(orderly_location_pull_metadata(root = here))
   expect_length(res$messages, 2)