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.

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

add space parameter for Posit Cloud #983

Merged
merged 10 commits into from
Sep 12, 2023
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# rsconnect (development version)

* Added `space` parameter to deploy directly to a space in Posit Cloud.

* Fixed analysis of directories that were smaller than the
`rsconnect.max.bundle.files=10000` limit but larger than the
`renv.config.dependencies.limit=1000` limit. (#968)
Expand Down
37 changes: 25 additions & 12 deletions R/client-cloud.R
Original file line number Diff line number Diff line change
Expand Up @@ -161,23 +161,27 @@ cloudClient <- function(service, authInfo) {
GET(service, authInfo, path, query)
},

createApplication = function(name, title, template, accountId, appMode) {
createApplication = function(name, title, template, accountId, appMode, spaceId = NULL) {
json <- list()
json$name <- name
json$application_type <- if (appMode %in% c("rmd-static", "quarto-static", "static")) "static" else "connect"
if (appMode %in% c("rmd-static", "quarto-static")) {
json$render_by <- "server"
}

currentProjectId <- getCurrentProjectId(service, authInfo)
# in case the source cloud project is a temporary copy, there is no
# content id. The output will be published without a space id.
if (!is.null(currentProjectId)) {
json$project <- currentProjectId
if (is.null(spaceId)) {
currentProjectId <- getCurrentProjectId(service, authInfo)
# in case the source cloud project is a temporary copy, there is no
# content id. The output will be published without a space id.
if (!is.null(currentProjectId)) {
json$project <- currentProjectId
Copy link
Contributor

Choose a reason for hiding this comment

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

This change does not add the json$project when provided an incoming spaceId. Is it needed in this case?

Compare this change to the change in deployApplication() -- there, the space associated with the current project is never used, while the current project id is used unconditionally (even when there is spaceId argument).

What is the expectation for the project and space fields provided to the different API calls? I'm not familiar with the APIs that are being called -- more details would be really helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change does not add the json$project when provided an incoming spaceId. Is it needed in this case?

Thanks, it is needed here – pushed a commit to change this logic. If we're executing inside a Posit Cloud project, we always want to link the output to that output through project. For the space, we want to update it if there's a specific space passed in through deployApp. If there's no space when we create the application, it should be set to the space of the project.


path <- paste0("/content/", currentProjectId)
currentProject <- GET(service, authInfo, path)
json$space <- currentProject$space_id
path <- paste0("/content/", currentProjectId)
currentProject <- GET(service, authInfo, path)
json$space <- currentProject$space_id
mslynch marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
json$space <- spaceId
}

output <- POST_JSON(service, authInfo, "/outputs", json)
Expand Down Expand Up @@ -230,12 +234,21 @@ cloudClient <- function(service, authInfo) {
revision$application_id
},

deployApplication = function(application, bundleId = NULL) {
deployApplication = function(application, bundleId = NULL, spaceId = NULL) {
mslynch marked this conversation as resolved.
Show resolved Hide resolved
outputPatchData <- list()

currentProjectId <- getCurrentProjectId(service, authInfo)
if (!is.null(currentProjectId)) {
outputPatchData$project <- currentProjectId
Copy link
Contributor

Choose a reason for hiding this comment

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

This is different logic than is used when creating; intentional? In this case, we could end up using currentProjectId from the environment and the spaceId argument. When creating, the spaceId argument is only used when there is no currentProjectId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the logic as-written makes sense (at least to me). When you deploy, it should definitely set the space based on the space of the environment project. But on redeploys, it would probably be more confusing for projects to move between spaces without providing the argument.

@m-- Any other thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also cc @kippandrew ^

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense to me.

}

if (!is.null(spaceId)) {
outputPatchData$space <- spaceId
}

if (length(outputPatchData) > 0) {
path <- paste0("/outputs/", application$id)
json <- list(project = currentProjectId)
PATCH_JSON(service, authInfo, path, json)
PATCH_JSON(service, authInfo, path, outputPatchData)
}

path <- paste0("/applications/", application$application_id, "/deploy")
Expand Down
2 changes: 1 addition & 1 deletion R/client-shinyapps.R
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ shinyAppsClient <- function(service, authInfo) {
GET(service, authInfo, path, query)
},

createApplication = function(name, title, template, accountId, appMode) {
createApplication = function(name, title, template, accountId, appMode, spaceId) {
mslynch marked this conversation as resolved.
Show resolved Hide resolved
json <- list()
json$name <- name
# the title field is only used on connect
Expand Down
12 changes: 9 additions & 3 deletions R/deployApp.R
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@
#' @param image Optional. The name of the image to use when building and
#' executing this content. If none is provided, Posit Connect will
#' attempt to choose an image based on the content requirements.
#' @param space Optional. For Posit Cloud, the id of the space where the content
#' should be deployed. If none is provided, content will be deployed to the
#' deploying user's workspace or deployed to the same space in case of
#' redeploy.
#' @examples
#' \dontrun{
#'
Expand Down Expand Up @@ -197,7 +201,8 @@ deployApp <- function(appDir = getwd(),
forceGeneratePythonEnvironment = FALSE,
quarto = NA,
appVisibility = NULL,
image = NULL
image = NULL,
space = NULL
) {

check_string(appDir)
Expand Down Expand Up @@ -365,7 +370,8 @@ deployApp <- function(appDir = getwd(),
target$appTitle,
"shiny",
accountDetails$accountId,
appMetadata$appMode
appMetadata$appMode,
space
)
taskComplete(quiet, "Created application with id {.val {application$id}}")
} else {
Expand Down Expand Up @@ -453,7 +459,7 @@ deployApp <- function(appDir = getwd(),
if (!quiet) {
cli::cli_rule("Deploying to server")
}
task <- client$deployApplication(application, bundle$id)
task <- client$deployApplication(application, bundle$id, space)
taskId <- if (is.null(task$task_id)) task$id else task$task_id
# wait for the deployment to complete (will raise an error if it can't)
response <- client$waitForTask(taskId, quiet)
Expand Down
8 changes: 7 additions & 1 deletion man/deployApp.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 46 additions & 1 deletion tests/testthat/test-client-cloud.R
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,50 @@ test_that("Create application", {
expect_equal(app$url, "http://fake-url.test.me/")
})

test_that("Create application with space id", {
mockServer <- mockServerFactory(list(
"^POST /outputs" = list(
content = function(methodAndPath, match, contentFile, ...) {
content <- jsonlite::fromJSON(readChar(contentFile, file.info(contentFile)$size))
expect_equal(content$application_type, "connect")
expect_equal(content$space, 333)
list(
"id" = 1,
"source_id" = 2,
"url" = "http://fake-url.test.me/",
"state" = "active"
)
}
),
"^GET /applications/([0-9]+)" = list(
content = function(methodAndPath, match, ...) {
end <- attr(match, "match.length")[2] + match[2]
application_id <- strtoi(substr(methodAndPath, match[2], end))

list(
"id" = application_id,
"content_id" = 1
)
})
))

restoreOpt <- options(rsconnect.http = mockServer$impl)
mslynch marked this conversation as resolved.
Show resolved Hide resolved
withr::defer(options(restoreOpt))

fakeService <- list(
protocol = "test",
host = "unit-test",
port = 42
)
client <- cloudClient(fakeService, NULL)

app <- client$createApplication("test app", "unused?", "unused?", "unused?", "shiny", 333)

expect_equal(app$id, 1)
expect_equal(app$application_id, 2)
expect_equal(app$url, "http://fake-url.test.me/")
})

test_that("Create static application", {
mockServer <- mockServerFactory(list(
"^POST /outputs" = list(
Expand Down Expand Up @@ -408,6 +452,7 @@ test_that("deployApplication updates the parent project", {
content = function(methodAndPath, match, contentFile, ...) {
content <- jsonlite::fromJSON(readChar(contentFile, file.info(contentFile)$size))
expect_equal(content$project, 41)
expect_equal(content$space, 333)
list(
"id" = 41
)
Expand Down Expand Up @@ -442,7 +487,7 @@ test_that("deployApplication updates the parent project", {
"id" = 100,
"application_id" = 101
)
client$deployApplication(application)
client$deployApplication(application, spaceId = 333)
})

test_that("Create static RMD application", {
Expand Down