Deployment record improvements #827
Conversation
There was a problem hiding this comment.
The envVars and version fields are new and should not cause much difficulty if folks roll-back to an old release; old rsconnect will parse those fields, but may remove them on write.
Should we add something to NEWS about the consequences to stored envVars if folks upgrade then downgrade?
https://github.com/rstudio/rsconnect/blob/v0.8.29/R/deployments.R
| @@ -69,6 +69,14 @@ test_that("can read/write metadata", { | |||
| expect_equal(out$meta2, "two") | |||
| }) | |||
|
|
|||
| test_that("can read/write version", { | |||
There was a problem hiding this comment.
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.
| path <- addTestDeployment(app, "test1", envVars = NULL) | ||
| deps <- deployments(app, excludeOrphaned = FALSE) | ||
| expect_equal(deps$envVars, list(character())) | ||
| expect_false("envVars" %in% rownames(read.dcf(path))) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
envVarshandling. Fixes Re-deploying old app fails because ofenvVars#820.version. Fixes Version the deployment dcf record #821.createDeploymentTarget()calls