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

Deployment record improvements #827

Merged
merged 3 commits into from
May 2, 2023
Merged

Deployment record improvements #827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d09c5f7
Deployment record improvements
hadley May 1, 2023
08d6d47
More informative variable name
hadley May 2, 2023
ecad833
Improve empty writing tests
hadley May 2, 2023
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
47 changes: 25 additions & 22 deletions R/deploymentTarget.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ deploymentTarget <- function(recordPath = ".",
}

createDeploymentTarget(
appName,
appTitle,
appId,
envVars,
fullAccount$name, # first deploy must be to own account
fullAccount$name,
fullAccount$server
appName = appName,
appTitle = appTitle,
appId = appId,
envVars = envVars,
username = fullAccount$name, # first deploy must be to own account
account = fullAccount$name,
server = fullAccount$server
)
} else if (nrow(appDeployments) == 1) {
# If both appName and appId supplied, check that they're consistent.
Expand Down Expand Up @@ -77,12 +77,12 @@ deploymentTargetForApp <- function(appId,
application <- getApplication(accountDetails$name, accountDetails$server, appId)

createDeploymentTarget(
application$name,
application$title %||% appTitle,
application$id,
application$owner_username,
accountDetails$name,
accountDetails$server
appName = application$name,
appTitle = application$title %||% appTitle,
appId = application$id,
username = application$owner_username,
account = accountDetails$name,
server = accountDetails$server
)
}

Expand All @@ -92,28 +92,31 @@ createDeploymentTarget <- function(appName,
envVars,
username,
account,
server) {
server,
version = 1) {
list(
appName = appName,
appTitle = appTitle %||% "",
envVars = envVars,
appId = appId,
username = username,
account = account,
server = server
server = server,
version = version
)
}

updateDeploymentTarget <- function(previous, appTitle = NULL, envVars = NULL) {
createDeploymentTarget(
previous$name,
appTitle %||% previous$title,
previous$appId,
envVars %||% previous$envVars[[1]],
appName = previous$name,
appTitle = appTitle %||% previous$title,
appId = previous$appId,
envVars = envVars %||% previous$envVars[[1]],
# if username not previously recorded, use current account
previous$username %||% previous$account,
previous$account,
previous$server
username = previous$username %||% previous$account,
account = previous$account,
server = previous$server,
version = previous$version
)
}

Expand Down
14 changes: 10 additions & 4 deletions R/deployments.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ deployments <- function(appPath = ".",
ok <- ok & okServer
}

deployments$envVars[is.na(deployments$envVars)] <- ""
if (is.character(deployments$envVars)) {
deployments$envVars <- strsplit(deployments$envVars, ", ")
}
Expand All @@ -79,9 +80,11 @@ deployments <- function(appPath = ".",

deploymentFields <- c(
"name", "title", "username", "account", "server", "hostUrl", "appId",
"bundleId", "url", "envVars"
"bundleId", "url", "envVars", "version"
)

deploymentRecordVersion <- 1L

saveDeployment <- function(recordDir,
target,
application,
Expand All @@ -97,6 +100,7 @@ saveDeployment <- function(recordDir,
account = target$account,
server = target$server,
envVars = target$envVars,
version = target$version,
hostUrl = hostUrl,
appId = application$id,
bundleId = bundleId,
Expand All @@ -111,7 +115,7 @@ saveDeployment <- function(recordDir,
addToDeploymentHistory(recordDir, deployment)
}

invisible(NULL)
invisible(path)
}

deploymentRecord <- function(name,
Expand All @@ -124,6 +128,7 @@ deploymentRecord <- function(name,
appId = NULL,
bundleId = NULL,
url = NULL,
version = deploymentRecordVersion,
metadata = list()) {

check_character(envVars, allow_null = TRUE)
Expand All @@ -134,11 +139,12 @@ deploymentRecord <- function(name,
username = username,
account = account,
server = server,
envVars = paste0(envVars, collapse = ", "),
envVars = if (length(envVars) > 0) paste0(envVars, collapse = ", ") else NA,
hostUrl = hostUrl %||% "",
appId = appId %||% "",
bundleId = bundleId %||% "",
url = url %||% ""
url = url %||% "",
version = version
)
c(standard, metadata)
}
Expand Down
5 changes: 3 additions & 2 deletions tests/testthat/helper.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ addTestDeployment <- function(path,
server = "example.com",
url = paste0("https://", server, "/", username, "/", appId),
hostUrl = NULL,
version = deploymentRecordVersion,
metadata = list()) {
saveDeployment(
path,
Expand All @@ -93,12 +94,12 @@ addTestDeployment <- function(path,
envVars = envVars,
account = account,
username = username,
server = server
server = server,
version = version
),
application = list(id = appId, url = url),
hostUrl = hostUrl,
metadata = metadata,
addToHistory = FALSE
)
invisible()
}
9 changes: 8 additions & 1 deletion tests/testthat/test-deploymentTarget.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,18 @@ test_that("succeeds if there's a single existing deployment", {
addTestAccount("ron")

app_dir <- withr::local_tempdir()
addTestDeployment(app_dir, appName = "test", appId = "1", username = "ron")
addTestDeployment(
app_dir,
appName = "test",
appId = "1",
username = "ron",
version = "999"
)

target <- deploymentTarget(app_dir)
expect_equal(target$appId, "1")
expect_equal(target$username, "ron")
expect_equal(target$version, "999")

target <- deploymentTarget(app_dir, appName = "test")
expect_equal(target$appId, "1")
Expand Down
36 changes: 32 additions & 4 deletions tests/testthat/test-deployments.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,25 @@ test_that("can read/write metadata", {
expect_equal(out$meta2, "two")
})

test_that("can read/write version", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we test reading a DCF file that lacks the envVars and version fields?

We should test reading single files with/without these fields as well as a collection of files where some of them lack these fields.

dir <- local_temp_app()

addTestDeployment(dir, version = "999")
out <- deployments(dir, excludeOrphaned = FALSE)
expect_equal(out$version, "999")
})

test_that("can read/write missing version", {
# also tests we can read files written by previous versions of package
dir <- local_temp_app()

path <- addTestDeployment(dir, version = NA)
out <- deployments(dir, excludeOrphaned = FALSE)

expect_false("version" %in% rownames(read.dcf(path)))
expect_equal(out$version, NA)
})

test_that("can read/write env vars", {
app <- local_temp_app()
addTestDeployment(app, "test1", envVars = c("TEST1", "TEST2"))
Expand All @@ -78,12 +97,21 @@ test_that("can read/write env vars", {
expect_equal(deps$envVars, list(c("TEST1", "TEST2"), character()))
})

test_that("can read env vars when none set", {
test_that("can read/write empty env vars", {
# also tests we can read files written by previous versions of package
app <- local_temp_app()
addTestDeployment(app, "test1", envVars = NA_character_)

# With empty character vector
path <- addTestDeployment(app, "test1", envVars = character())
deps <- deployments(app, excludeOrphaned = FALSE)
expect_equal(deps$envVars, list(NA_character_))
expect_false("envVars" %in% rownames(read.dcf(path)))
Copy link
Member Author

Choose a reason for hiding this comment

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

@aronatkins this is where we test what happens when we deliberately create a deployment without envVars — we test that envVars is not written to the DCF, but it's still successfully read. I'll add something similar for version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I missed that this test was confirming the processing of an "old" DCF. Would you mind adding a comment / renaming the test to remind us that this is testing DCF compatibility upon upgrade?

expect_equal(deps$envVars, list(character()))

# Or with empty string
path <- addTestDeployment(app, "test1", envVars = "")
deps <- deployments(app, excludeOrphaned = FALSE)
expect_false("envVars" %in% rownames(read.dcf(path)))
expect_equal(deps$envVars, list(character()))
})

test_that("can read/write env vars", {
Expand All @@ -108,7 +136,7 @@ test_that("saveDeployment appends to global history", {
appName = "my-app",
appTitle = "",
appId = 10,
envVars = NULL,
envVars = "abc", # ensure there's an envVars column in output
account = "foo",
username = "foo",
server = "bar"
Expand Down