Skip to content
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.

Sign up for GitHub

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

chore: use v1 timezones API #315

Merged
merged 15 commits into from
Oct 23, 2024
Merged

chore: use v1 timezones API #315

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8ad8eb9
first pass at new time zone URL
toph-allen Oct 11, 2024
823862e
server version updates
toph-allen Oct 17, 2024
7762c7a
refactor error_if_less_than to use version number
toph-allen Oct 17, 2024
eed331f
add tests for timezones using subclasses to mock
toph-allen Oct 21, 2024
6918af8
simplify function, update tests
toph-allen Oct 22, 2024
18e2c8e
formatting changes
toph-allen Oct 22, 2024
d31c496
refactor tests
toph-allen Oct 22, 2024
3fb399f
clean up
toph-allen Oct 22, 2024
5ffd793
documentation
toph-allen Oct 22, 2024
adc2041
news
toph-allen Oct 22, 2024
1fb56f4
cleanup
toph-allen Oct 23, 2024
4e7bb6d
integration test
toph-allen Oct 23, 2024
de718e7
disable integration test
toph-allen Oct 23, 2024
b2488ed
Update tests/testthat/test-schedule.R
toph-allen Oct 23, 2024
480fb0f
in-line mocks now fully specify route with verb
toph-allen Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions R/connect.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#' @family R6 classes
#'
#' @export
Connect <- R6::R6Class(

Check warning on line 23 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=23,col=1,[cyclocomp_linter] Functions should have cyclomatic complexity of less than 15, this has 27.

Check warning on line 23 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=23,col=1,[object_name_linter] Variable and function name style should match snake_case or symbols.
"Connect",
public = list(
#' @field server The base URL of your Posit Connect server.
Expand Down Expand Up @@ -67,7 +67,7 @@
print = function(...) {
cat("Posit Connect API Client: \n")
cat(" Posit Connect Server: ", self$server, "\n", sep = "")
cat(" Posit Connect API Key: ", paste0(strrep("*", 11), substr(self$api_key, nchar(self$api_key) - 3, nchar(self$api_key))), "\n", sep = "")

Check warning on line 70 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=70,col=101,[line_length_linter] Lines should not be more than 100 characters. This line is 147 characters.
# TODO: something about API key... role... ?
# TODO: point to docs on methods... how to see methods?
cat("\n")
Expand Down Expand Up @@ -187,7 +187,7 @@
self$request("HEAD", url, parser = NULL, ...)
},

#' @description Perform an HTTP DELETE request of the named API path. Returns the HTTP response object.

Check warning on line 190 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=190,col=101,[line_length_linter] Lines should not be more than 100 characters. This line is 107 characters.
#' @param path API path relative to the server's `/__api__` root.
#' @param ... Arguments to `httr::DELETE()`
#' @param url Target URL. Default uses `path`, but provide `url` to request
Expand Down Expand Up @@ -252,7 +252,7 @@
#' @description Return all tags.
#' @param use_cache Indicates that a cached set of tags is used.
get_tags = function(use_cache = FALSE) {
error_if_less_than(self, "1.8.6")
error_if_less_than(self$version, "1.8.6")
# TODO: check cache "age"?
if (is.null(self$tags) || !use_cache) {
self$tags <- self$tag()
Expand Down Expand Up @@ -305,7 +305,7 @@
#' @param name The tag name.
#' @param parent_id The parent identifier.
tag_create = function(name, parent_id = NULL) {
error_if_less_than(self, "1.8.6")
error_if_less_than(self$version, "1.8.6")
dat <- list(
name = name
)
Expand All @@ -328,7 +328,7 @@
#' @description Get a tag.
#' @param id The tag identifier.
tag = function(id = NULL) {
error_if_less_than(self, "1.8.6")
error_if_less_than(self$version, "1.8.6")
if (is.null(id)) {
path <- v1_url("tags")
} else {
Expand Down Expand Up @@ -358,7 +358,7 @@
count = min(page_size, .limit)
)
if (!is.null(filter)) {
query$filter <- paste(sapply(1:length(filter), function(i) {

Check warning on line 361 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=361,col=38,[seq_linter] 1:length(...) is likely to be wrong in the empty edge case. Use seq_along(...) instead.
sprintf("%s:%s", names(filter)[i], filter[[i]])
}), collapse = .collapse)
}
Expand Down Expand Up @@ -751,7 +751,7 @@
warn_experimental("repo_account")
parsed_url <- httr::parse_url(host)
if (is.null(parsed_url$scheme) || is.null(parsed_url$hostname)) {
stop(glue::glue("Scheme and hostname must be provided (i.e. 'https://github.com'). You provided '{host}'"))

Check warning on line 754 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=754,col=101,[line_length_linter] Lines should not be more than 100 characters. This line is 115 characters.
}
host <- glue::glue(parsed_url$scheme, "://", parsed_url$hostname)
path <- unversioned_url("repo", "account")
Expand Down Expand Up @@ -781,7 +781,7 @@
#' @param start Starting time.
#' @param end Ending time.
#' @param detailed Indicates detailed schedule information.
schedules = function(start = Sys.time(), end = Sys.time() + 60 * 60 * 24 * 7, detailed = FALSE) {

Check warning on line 784 in R/connect.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/connect.R,line=784,col=101,[line_length_linter] Lines should not be more than 100 characters. This line is 101 characters.
warn_experimental("schedules")
url <- v1_url("experimental", "schedules")
query_params <- rlang::list2(
Expand Down Expand Up @@ -833,6 +833,17 @@
}

# end --------------------------------------------------------
),
private = list(
.version = NULL
),
active = list(
version = function() {
if (is.null(private$.version)) {
private$.version <- safe_server_version(self)
}
private$.version
}
)
)

Expand Down Expand Up @@ -886,7 +897,7 @@
tryCatch(
{
check_connect_license(con)
check_connect_version(using_version = safe_server_version(con))
warn_untested_connect(using_version = safe_server_version(con))
},
error = function(err) {
if (.check_is_fatal) {
Expand Down
2 changes: 1 addition & 1 deletion R/content.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#'
#' @family R6 classes
#' @export
Content <- R6::R6Class(

Check warning on line 7 in R/content.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/content.R,line=7,col=1,[object_name_linter] Variable and function name style should match snake_case or symbols.
"Content",
public = list(
#' @field connect An R6 Connect object
Expand Down Expand Up @@ -45,7 +45,7 @@
#' @param bundle_id The bundle identifer.
#' @param filename Where to write the result.
#' @param overwrite Overwrite an existing filename.
bundle_download = function(bundle_id, filename = tempfile(pattern = "bundle", fileext = ".tar.gz"), overwrite = FALSE) {

Check warning on line 48 in R/content.R

View workflow job for this annotation

GitHub Actions / lint 

file=R/content.R,line=48,col=101,[line_length_linter] Lines should not be more than 100 characters. This line is 124 characters.
url <- v1_url("content", self$get_content()$guid, "bundles", bundle_id, "download")
self$get_connect()$GET(url, httr::write_disk(filename, overwrite = overwrite), parser = "raw")
return(filename)
Expand All @@ -65,7 +65,7 @@
#' @param ... Content fields.
update = function(...) {
con <- self$get_connect()
error_if_less_than(con, "1.8.6")
error_if_less_than(con$version, "1.8.6")
url <- v1_url("content", self$get_content()$guid)
body <- rlang::list2(...)
if (length(body)) {
Expand Down
6 changes: 3 additions & 3 deletions R/deploy.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ deploy_current <- function(content) {
set_vanity_url <- function(content, url, force = FALSE) {
validate_R6_class(content, "Content")
con <- content$get_connect()
error_if_less_than(con, "1.8.6")
error_if_less_than(con$version, "1.8.6")
guid <- content$get_content()$guid

scoped_experimental_silence()
Expand All @@ -449,7 +449,7 @@ set_vanity_url <- function(content, url, force = FALSE) {
#' @export
delete_vanity_url <- function(content) {
con <- content$get_connect()
error_if_less_than(con, "1.8.6")
error_if_less_than(con$version, "1.8.6")
guid <- content$get_content()$guid

con$DELETE(v1_url("content", guid, "vanity"))
Expand All @@ -470,7 +470,7 @@ delete_vanity_url <- function(content) {
get_vanity_url <- function(content) {
validate_R6_class(content, "Content")
con <- content$get_connect()
error_if_less_than(con, "1.8.6")
error_if_less_than(con$version, "1.8.6")
guid <- content$get_content()$guid

van <- tryCatch(
Expand Down
8 changes: 7 additions & 1 deletion R/schedule.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,13 @@ schedule_describe <- function(.schedule) {
#' @family schedule functions
#' @export
get_timezones <- function(connect) {
raw_tz <- connect$GET(unversioned_url("timezones"))
raw_tz <- tryCatch(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonkeane One problem using tryCatch is that connectapi emits a message, not just an error, when it hits an HTTP error. This means that even if we catch the error, a message is emitted saying 404 page not found when the incorrect URL is tried. This may have been our original motivation for not parsing the responses in the thumbnail API. I'll add a comment to this thread with a code ref for the error handling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rstudio/connectapi/blob/adc20411960200bf6d4f6ea8bf64d727ead5d511/R/connect.R#L79-L96

Copy link
Collaborator Author

@toph-allen toph-allen Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and create an issue to track this so we don't need to treat it as blocking this work.

[EDIT] #317

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toph-allen and I discussed this over Zoom. Since it isn't directly relevant to this feature, we're going to create a separate issue to capture the oddities.

Generally speaking, a console write should always be accomplished through a logging interface. That way the user has some control over the messages (e.g.., disabling them). And even then, a logging interface is usually overkill. In my opinion, this type of message should just be part of the raised error. Then the program can handle the error as needed.

connect$GET(v1_url("timezones")),
error = function(e) {
connect$GET(unversioned_url("timezones"))
}
)

tz_values <- purrr::map_chr(raw_tz, ~ .x[["timezone"]])
tz_display <- purrr::map_chr(raw_tz, ~ glue::glue("{.x[['timezone']]} ({.x[['offset']]})"))

Expand Down
30 changes: 18 additions & 12 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,20 +134,25 @@ safe_server_version <- function(client) {
version <- safe_server_settings(client)$version
if (is.null(version) || nchar(version) == 0) {
message("Version information is not exposed by this Posit Connect instance.")
# Return 0 so this will always show up as "too old"
version <- "0"
version <- NA
}
version
}

error_if_less_than <- function(client, tested_version) {
server_version <- safe_server_version(client)
comp <- compare_connect_version(server_version, tested_version)
if (comp < 0) {
msg <- paste0(
"ERROR: This API requires Posit Connect version ", tested_version,
" but you are using", server_version, ". Please use a previous version",
" of the `connectapi` package, upgrade Posit Connect, or review the API ",
error_if_less_than <- function(client_version, tested_version) {
comp <- compare_connect_version(client_version, tested_version)
if (is.na(comp)) {
msg <- glue::glue(
"WARNING: This API requires Posit Connect version {tested_version} ",
"but the server version is not exposed by this Posit Connect instance. ",
"You may be experience errors when using this functionality."
)
warn_once(msg)
} else if (comp < 0) {
msg <- glue::glue(
"ERROR: This API requires Posit Connect version {tested_version} ",
"but you are using {client_version}. Please use a previous version ",
"of the `connectapi` package, upgrade Posit Connect, or review the API ",
"documentation corresponding to your version."
)
stop(msg)
Expand All @@ -156,12 +161,13 @@ error_if_less_than <- function(client, tested_version) {
}

compare_connect_version <- function(using_version, tested_version) {
if (is.na(using_version)) return(NA)
compareVersion(simplify_version(using_version), simplify_version(tested_version))
}

check_connect_version <- function(using_version, minimum_tested_version = "1.8.8.2") {
warn_untested_connect <- function(using_version, minimum_tested_version = "1.8.8.2") {
comp <- compare_connect_version(using_version, minimum_tested_version)
if (comp < 0) {
if (isTRUE(comp < 0)) {
warn_once(glue::glue(
"You are using an older version of Posit Connect ",
"({using_version}) than is tested ({minimum_tested_version}). ",
Expand Down
1 change: 0 additions & 1 deletion tests/testthat/2024.08.0/__api__/server_settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
{
"version": "2024.06.0"
}
7 changes: 3 additions & 4 deletions tests/testthat/2024.09.0/__api__/server_settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
{
"hostname": "dogfood01",
"version": "2024.09.0-dev+77-g9296f96ffb",
"build": "v2024.08.0-77-g9296f96ffb",
"about": "Posit Connect v2024.09.0-dev+77-g9296f96ffb",
"hostname": "example",
"version": "2024.09.0",
"about": "Posit Connect v2024.09.0",
"authentication": {
"handles_credentials": false,
"handles_login": true,
Expand Down
Binary file removed tests/testthat/Rplots.pdf
View file
Open in desktop
Binary file not shown.
6 changes: 3 additions & 3 deletions tests/testthat/_snaps/utils.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
# check_connect_version warning snapshot
# warn_untested_connect warning snapshot

Code
capture_warning(check_connect_version("2022.02", "2022.01"))
capture_warning(warn_untested_connect("2022.02", "2022.01"))
Output
NULL

---

Code
capture_warning(check_connect_version("2022.01", "2022.02"))
capture_warning(warn_untested_connect("2022.01", "2022.02"))
Output
<warning/rlang_warning>
Warning:
Expand Down
14 changes: 14 additions & 0 deletions tests/testthat/test-connect.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,18 @@ with_mock_api({
"MY_MAGIC_HEADER: value"
)
})


test_that("client$version is NA when server settings lacks version info", {
con <- Connect$new(server = "https://connect.example", api_key = "fake")
expect_message(v <- con$version, "Version information is not exposed by this Posit Connect instance")
expect_true(is.na(v))
})
})

test_that("client$version is returns version when server settings exposes it", {
with_mock_dir("2024.09.0", {
con <- Connect$new(server = "https://connect.example", api_key = "fake")
expect_equal(con$version, "2024.09.0")
})
})
Loading
Loading