Cloud static outputs #825

mslynch · 2023-04-28T16:40:56Z

Add support for deploying client-rendered static outputs to Posit Cloud
Save Cloud deployments with appId as the Cloud content id (instead of application id)
Introduce versioning to dcf files (needed to support backwards compatibility for old Cloud dcf files saved with appId as the application id

Resolves #824.
Resolves #821.

* Basic way to test with mocked http * Tests for cloud deploys of connect and static apps * Provide a way for individual tests to add responses

If output is in trashed or archived state, set it to active

…t ids

cloud static outputs branch

R/client-cloud.R

…on cases

hadley

Looking good! Do you also want to add a news bullet briefly describing these changes?

R/deploymentTarget.R

R/deployments.R

R/http.R

R/client-cloud.R

R/deployApp.R

R/deploymentTarget.R

aronatkins · 2023-05-01T12:51:31Z

R/deployments.R

@@ -143,7 +143,10 @@ deploymentRecord <- function(name,
  c(standard, metadata)
 }

+dcfVersion <- "1"


@hadley You had earlier decided to not make structural changes to the DCF files, making for simpler roll-forward/roll-back. Has your position changed?

In particular, how would the current CRAN release react to a versioned deployment DCF file?

It should just ignore it; the metadata argument has always allowed you to add arbitrary additional columns which don't otherwise cause problems.

The meaning of one of the fields is changing, yes? What does this mean when we attempt to use those DCF files with an old rsconnect release?

Ooh, I think I get what you mean now — we're adding a version field that will allow us to do more versioning in the future, but there's no intent to add any backward incompatible changes at this time. The current goal is to make it possible to tell if applicationId means appId or contentId in existing deployments to cloud; contentId is substantially easier to work with and more meaningful on the server side, but we don't want to break existing deployment records that stored appId.

We considered renaming applicationId more generally (since rsconnect is not just about applications any more), but given that it's mostly an internal feature used by the IDE, we didn't think it was worth it.

The situation that concerned me:

Deploy (new) using CRAN rsconnect (or any version prior to this change).

Re-deploy using this version of rsconnect (which upgrades the DCF file).

Re-deploy using CRAN rsconnect.

Is the deployment DCF value of applicationId constant or does it change during this re-deploy sequence?

@aronatkins The value of appId in the DCF file would change when deploying to cloud using this version of rsconnect (from the application id to the content id), so that scenario would break at step 3.

@hadley - You had mentioned trying to avoid this type of backwards incompatibility with the upcoming release.. has your opinion changed?

The reasoning: There's enough changing in rsconnect that the upcoming release will cause issues for some folks despite our efforts to avoid that. Folks will probably downgrade to the previous CRAN release until we can patch. We wanted to avoid situations where the downgrade caused its own problems.

I think this is ok because of its relatively limited scope — my other proposed change would've broken every single piece of metadata that you'd saved, which seemed a bit extreme.

R/deployments.R

tests/testthat/test-deployments.R

Co-authored-by: Hadley Wickham <hadley@posit.co>

…s id and application id as application_id

… removing backwards compatibility

…tent support

R/client-cloud.R

toph-allen

I can't actually see where deploymentRecordVersion is set. It looks like there used to be a variable named dcfVersion in deployments.R, but that change was removed in one of the commits that merged main into this branch.

I might just be overlooking something or searching for the wrong string, or misunderstanding the current state of things looking at the conversation. :)

mslynch · 2023-05-03T14:57:18Z

R/deploymentTarget.R

@@ -94,7 +94,7 @@ createDeploymentTarget <- function(appName,
                                   username,
                                   account,
                                   server,
-                                   version = 1) {
+                                   version = deploymentRecordVersion) {


I can't actually see where deploymentRecordVersion is set.

@toph-allen That's right here.

hadley

Looks good to me!

The only last thing you might want to do is to note any manual testing you did in a new .Rmd in tests/manual, so it's easier to remember in the future.

aronatkins

The wiring from application$id to application$application_id and changing the meaning of application$id is bound to throw us off in the future...

NEWS.md

aronatkins · 2023-05-08T16:27:54Z

R/client-cloud.R

+    getApplication = function(outputOrApplicationId, deploymentRecordVersion) {
+      if (is.na(deploymentRecordVersion)) {
+        # In pre-versioned dcf files, outputOrApplicationId is the id of the application.
+        # TODO: consider removing support for this case a year after the release of 0.8.30


Please update this TODO; the next release will be rsconnect-1.0.0.
How will we decide that it is "safe" to remove this block?

It's never quite "safe" to remove this block – as long as someone has an old has an old deployment DCF file, this would break for them on removal. We don't have visibility into that, but I'm assuming we don't want to support this case forever.

aronatkins · 2023-05-08T16:37:21Z

R/deployApp.R

+        application <- client$getApplication(target$appId, target$version)
+        taskComplete(quiet, "Found application {.url {application$url}}")
+
+        if (application$type == "static") {


This "static" notion is specific to Posit Cloud; does it belong here or with the call to uploadCloudBundle (where we already have cloud-specific logic)?

Related: Do we also need to call createRevision in the 404 path just below? applicationDeleted calls createApplication -- could you explain why this path explicitly creates a revision while the creation path does not?

This "static" notion is specific to Posit Cloud; does it belong here or with the call to uploadCloudBundle (where we already have cloud-specific logic)?

I think it really should go here because it creates the application. I originally had it there, but Hadley left a comment and convinced me otherwise.

Related: Do we also need to call createRevision in the 404 path just below? applicationDeleted calls createApplication -- could you explain why this path explicitly creates a revision while the creation path does not?

We don't – for static content, the revision is created implicitly when we create the application.

OK. Thanks.

…readers

mslynch and others added 15 commits March 21, 2023 16:28

initial implementation for static outputs in cloud

e8a9364

save content id for cloud applications in dcf file's applicationId

0b3096d

better json-ing

5a931d4

make deploys and redeploys work for static applications

871519e

remove bad argument

31ccaed

revert change to give deploymentTargetForApp appId a default argument

4b32d80

Merge branch 'main' into cloud-static-outputs

51daf95

remove extraneous character in logging

1675271

save appId differently for cloud vs connect

42cd94a

Add unit tests of cloud functionality (#3)

c560e9f

* Basic way to test with mocked http * Tests for cloud deploys of connect and static apps * Provide a way for individual tests to add responses

If output is in trashed or archived state, set it to active

e764dbc

Merge pull request #5 from mslynch/handle-trashed-archived

ef39cc0

If output is in trashed or archived state, set it to active

version dcf files to distinguish between cloud application and conten…

629a6bd

…t ids

Merge branch 'main' into cloud-static-outputs

ee06339

Merge pull request #823 from mslynch/cloud-static-outputs

67d8221

cloud static outputs branch

mslynch marked this pull request as draft April 28, 2023 16:41

mslynch mentioned this pull request Apr 28, 2023

initial implementation for static outputs in cloud mslynch/rsconnect#2

Closed

appease linter

3959197

mslynch marked this pull request as ready for review April 28, 2023 17:26

mslynch requested review from hadley, aronatkins, mbaynton and omar-rs April 28, 2023 17:26

omar-rs reviewed Apr 28, 2023

View reviewed changes

R/client-cloud.R Outdated Show resolved Hide resolved

omar-rs reviewed Apr 28, 2023

View reviewed changes

R/client-cloud.R Outdated Show resolved Hide resolved

rename cloud client getApplication id parameter and clarify dcf versi…

3a31ee5

…on cases

hadley reviewed Apr 28, 2023

View reviewed changes

aronatkins reviewed May 1, 2023

View reviewed changes

mslynch added 2 commits May 1, 2023 09:34

name arguments in http.R

2c6ec3f

check solely for dcfVersion being NA

23f7018

mslynch and others added 9 commits May 1, 2023 09:44

use %||% instead of ifelse to determine output_id

bb81aa3

Co-authored-by: Hadley Wickham <hadley@posit.co>

refactor revision creation to be closer to application creation

7e068b4

use regular if/else to determine application_type

c995cad

in cloud client createApplication/getApplication, return content id a…

ac360e6

…s id and application id as application_id

always use application$id in saveDeployment

61abe5b

update client-cloud.R getApplication with note about when to consider…

30b5d5f

… removing backwards compatibility

use paste0 instead of paste in cloud-client.R

de5ad91

set default value for dcfFile argument

730a30c

add news bullets about DCF version, Cloud appId, and Cloud static con…

e01eb2a

…tent support

mbaynton reviewed May 2, 2023

View reviewed changes

R/client-cloud.R Outdated Show resolved Hide resolved

mslynch added 4 commits May 2, 2023 10:12

Merge branch 'main' into cloud-static-outputs

215fcbb

state that deployApp also supports cloud static content in news bullet

ab98e61

Merge branch 'main' into cloud-static-outputs

8b817aa

Merge branch 'main' into cloud-static-outputs

0ceb441

toph-allen reviewed May 2, 2023

View reviewed changes

R/client-cloud.R Outdated Show resolved Hide resolved

toph-allen reviewed May 2, 2023

View reviewed changes

mslynch commented May 3, 2023

View reviewed changes

correct variable name on comment explaining outputOrApplicationId

4fc6ee9

omar-rs approved these changes May 3, 2023

View reviewed changes

mslynch requested review from hadley and aronatkins May 5, 2023 16:27

hadley approved these changes May 8, 2023

View reviewed changes

aronatkins approved these changes May 8, 2023

View reviewed changes

mslynch added 4 commits May 8, 2023 13:01

Merge branch 'main' into cloud-static-outputs

8c7e5af

make news bullet about appId change more comprehensible to non-Posit …

dfe3284

…readers

update TODO with correct version of next release

75b39cd

Merge branch 'main' into cloud-static-outputs

a43176b

mslynch merged commit bf0e209 into main May 9, 2023

mslynch deleted the cloud-static-outputs branch May 9, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud static outputs #825

Cloud static outputs #825

mslynch commented Apr 28, 2023

hadley left a comment

aronatkins May 1, 2023

hadley May 1, 2023 •

edited

Loading

aronatkins May 1, 2023

hadley May 1, 2023

aronatkins May 1, 2023

mslynch May 1, 2023 •

edited

Loading

aronatkins May 1, 2023

aronatkins May 1, 2023

hadley May 2, 2023

toph-allen left a comment

mslynch May 3, 2023

hadley left a comment

aronatkins left a comment

aronatkins May 8, 2023

mslynch May 8, 2023

aronatkins May 8, 2023

mslynch May 8, 2023

aronatkins May 8, 2023

Cloud static outputs #825

Cloud static outputs #825

Conversation

mslynch commented Apr 28, 2023

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mslynch May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toph-allen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley left a comment

Choose a reason for hiding this comment

aronatkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley May 1, 2023 •

edited

Loading

mslynch May 1, 2023 •

edited

Loading