rstudio / pins-r

Pin, discover, and share resources
https://pins.rstudio.com
Other
312 stars 63 forks source link

`rsc_version` will not work under some configurations of RSC #467

Closed scottmmjackson closed 3 years ago

scottmmjackson commented 3 years ago

rsc_version currently queries RSC server settings for a version string and then attempts to parse with package_version. Two problems with this:

However, there's not really a better way to do feature detection of pins support for RSC. Therefore, what I'd suggest is turning this into a warning and informing the user that if a deployment failure occurs, that they might want to check their RSC version.

I wouldn't want to make version constraints opt-out, I think too many heavy users of RSC are hiding their version to make this reasonable.

Alternatively, we could trap for a blank version string and fix parsing connect dev build strings, but that doesn't harden us against connect changing its versioning again.

aronatkins commented 3 years ago

Here is the error that can happen when using a development version of Connect:

b <- board_rsconnect(account = "USER", server = "DEVELOPMENT")
# => Error: invalid version specification ‘1.8.8.3-dev312’

For version comparisons, it is safe to strip off everything after the -.

hadley commented 3 years ago

Do you know what happens if you deploy to a version of RSC that doesn't support pins? I assume it will complain about an unsupported content type, so maybe it's fine to just let that error message bubble up to the user? Do we expect to see many users of 1.7.7 or earlier in the wild?

scottmmjackson commented 3 years ago

I'm not sure, if I remember correctly it's a special type of static deployment so it's possible it could just deploy statically and be unusable afterwards. I can try to check on this later though.

aronatkins commented 3 years ago

Pins are static content (HTML and data files) to Connect. The pins package sets content_category="pin", which lets Connect filter for that content when folks ask to see just "pins". Additionally, we may use different icons/messages in the Connect dashboard for pins.

That said, the "pin" category does not alter how the content is processed or served. Pins (as static data) are supported by all versions of Connect. We prototyped serving the data files ahead of the existence of the pins package.

aronatkins commented 3 years ago

The 1.7.8 Connect release (dev version 1.7.7) was the first that declared support for pins:

  • RStudio Connect supports sharing of resources with the experimental pins package.

https://docs.rstudio.com/connect/news/#rstudio-connect-178

My earlier comments still hold; pins can be deployed to earlier Connect but without the filtering/UI changes.

scottmmjackson commented 3 years ago

Tried to deploy on 1.7.6 with the version check disabled and I got the following:

pin(c(1,2,3,4), "numbers", "some numbers", "rsconnect")
 Error in rsc_check_status(req) : Not Found (HTTP 404). 
16.
stop(http_condition(x, "error", task = task, call = call)) 
15.
httr::stop_for_status(req) at board_rsconnect.R#697
14.
rsc_check_status(req) at board_rsconnect.R#603
13.
rsc_GET(board, "v1/content", list(name = name$name)) at board_rsconnect.R#462
12.
rsc_content_find(board, name, warn = FALSE) at board_rsconnect.R#310
11.
doTryCatch(return(expr), name, parentenv, handler) 
10.
tryCatchOne(expr, names, parentenv, handlers[[1L]]) 
9.
tryCatchList(expr, classes, parentenv, handlers) 
8.
tryCatch({
    guid <- rsc_content_find(board, name, warn = FALSE)$guid
    rsc_content_update(board, guid, metadata, access_type = access_type)
    guid ... at board_rsconnect.R#308
7.
pin_store.pins_board_rsconnect(board = board, name = name, paths = path, 
    metadata = metadata, ...) at pin-store-fetch.R#25
6.
pin_store(board = board, name = name, paths = path, metadata = metadata, 
    ...) at board_rsconnect.R#412
5.
board_pin_create.pins_board_rsconnect(board, store_path, name = name, 
    metadata = metadata, ...) at legacy_board.R#18
4.
board_pin_create(board, store_path, name = name, metadata = metadata, 
    ...) at pin.R#106
3.
board_pin_store(board, path, name, metadata, ...) at pin.R#150
2.
pin.default(c(1, 2, 3, 4), "numbers", "some numbers", "rsconnect") at pin.R#42
1.
pin(c(1, 2, 3, 4), "numbers", "some numbers", "rsconnect") 

/v1/content was added in 1.8.8 which suggests that this version of pins requires 1.8.8 (?)

hadley commented 3 years ago

I think the pins policy should be to match RStudio's support version policy. Currently that means we need to support 1.8.6, and moving forward we need to support all versions released in the last 18 months. The best way to enforce/test for this will be to finish up #411.

hadley commented 3 years ago

Closing this since we're now testing with 1.8.6.2.

github-actions[bot] commented 2 years ago

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.