republishing and collaborating #1021

aronatkins · 2023-10-23T19:34:57Z

Revisit how the deployment target is selected.

Use 0.8.29 implementation when reviewing this work. In particular, that version of deploymentTarget() did not offer any separation between the "owning" account and the "deploying" account. Details recorded in the in-memory deployment record were not necessarily consistent with the on-disk deployment record. Additionally, resolving that deployment record to an actual remote application occurred much later, in applicationForTarget().
https://github.com/rstudio/rsconnect/blob/v0.8.29/R/deployApp.R

The implementation available in 1.0.0 until now did a good job consolidating this logic, but lost track of the fact that we may have a local user attempting to re-use deployment records created by some other user.

This change sees us use the following prioritized techniques to locate a deployment record:

When appId is provided, it must be used to locate the target content. A target (local) account is required to make this determination. When a local deployment record is not available, the application is loaded from the server. The target deployment record does not need to be owned by the local target account.
When appName is provided without appId, it must be used to locate the target content or create a new deployment target.
When neither appName nor appId is provided, the user is asked to choose from the available deployment records (which must correspond to a locally available account).
When there are no deployment records, a generated name is used along with a target account to remotely probe for that name (owned by the target account) before creating a new item.

Some collaboration instructions from the Connect user guide: https://docs.posit.co/connect/user/publishing/#publishing-collaboration

Related issues: #1019, #1013, #1007, #981 along with multiple internal reports.

This change may fix each of those issues, but they need more review to be sure.

…re work

prioritized set of deployment considerations: 1. appId used above all else, and may be owned by a non-local account. 2. appName can identify local or remote content, owned by a local account. 3. locally available deployments are preferred when other identifiers not provided. 4. fall-back on generated name (based on path) and user-chosen local account.

aronatkins

Will need NEWS ahead of merge.

R/applications.R

R/deploymentTarget.R

hadley · 2023-10-23T20:28:28Z

R/deploymentTarget.R

-    }
+# Discover the deployment target given appId.
+#
+# When appId is provided, all other information is secondary. An appId is an indication from the


I found this comment very useful for understand the problem and where I went wrong previously.

Thanks. It took so much code-reading and reasoning about the previous workflow to see it. I'm hoping that being much more explicit will help us in the future.

hadley · 2023-10-23T20:30:01Z

R/deploymentTarget.R

    appName = application$name,
    appTitle = application$title %||% appTitle,
    appId = application$id,
-    envVars = NULL,
+    envVars = envVars,
    username = application$owner_username %||% accountDetails$name,


Should we change the arguments to createDeploymentTarget() to more clearly differentiate the username of the current user vs. the username of the account owner? Might need to be in a separate PR to avoid cluttering this one, but I think it's likely to make understanding the code easier in the future.

The deployment record has the account+server corresponding to local account record for the user responsible for creating the deployment record on disk. This is usually the creator, but not always (e.g. collaborator publish using appId). The username is associated with the creating user (unsure if we get this back from shinyapps.io).

I'm not sure how to best represent this possible inconsistency. Happy to file an issue for further discussion.

I was thinking maybe we could rename username to something like owner_account to make it clear that this is also an account name, but it's the account name for the original author.

R/deploymentTarget.R

tests/testthat/_snaps/deploymentTarget.md

R/deployApp.R

R/applications.R

R/deployApp.R

R/deploymentTarget.R

aronatkins · 2023-10-23T21:32:44Z

R/deploymentTarget.R

-    }
+# Discover the deployment target given appId.
+#
+# When appId is provided, all other information is secondary. An appId is an indication from the


Thanks. It took so much code-reading and reasoning about the previous workflow to see it. I'm hoping that being much more explicit will help us in the future.

R/deploymentTarget.R

aronatkins · 2023-10-23T21:41:41Z

R/deploymentTarget.R

    appName = application$name,
    appTitle = application$title %||% appTitle,
    appId = application$id,
-    envVars = NULL,
+    envVars = envVars,
    username = application$owner_username %||% accountDetails$name,


The deployment record has the account+server corresponding to local account record for the user responsible for creating the deployment record on disk. This is usually the creator, but not always (e.g. collaborator publish using appId). The username is associated with the creating user (unsure if we get this back from shinyapps.io).

I'm not sure how to best represent this possible inconsistency. Happy to file an issue for further discussion.

R/deploymentTarget.R

tests/testthat/_snaps/deploymentTarget.md

R/deploymentTarget.R

… target

deploymentTarget ==> findDeploymentTarget deploymentTargetFromAppId ==> findDeploymentTargetByAppId deploymentTargetFromAppName ==> findDeploymentTargetByAppName updateDeploymentTarget ==> updateDeployment createDeploymentTarget ==> createDeployment

This rewrite showed that from-disk deployment records had different field names than the records created on-the-fly. In particular, "appName" and "appTitle" were used by the "deployment target", while "name" and "title" were used for on-disk deployment records. This made it challenging to use the same helpers. All deployment objects, either read from disk or created in-memory now use "name" and "title". This is a step towards always using deployment records.

…ror context

aronatkins · 2023-10-24T16:10:12Z

tests/testthat/test-deploymentTarget.R

+  ))
+})
+
+test_that("updateDeployment ignores NULL updates", {


Adding these tests let me see that we were mixing records from disk which used name and records created on-the-fly which used appName. We are now consistent.

hadley

Looks like a nice improvement to code clarity!

R/applications.R

R/deploymentTarget.R

R/deployments.R

toph-allen

Looks good to me. A lot clearer for posterity too.

R/deploymentTarget.R

tests/testthat/test-deploymentTarget.R

ChaitaC · 2023-11-01T05:04:56Z

Verified the fix for #1013

Before the fix, when using CRAN Version 1.1.1

── Preparing for deployment ────────────────────────────────────────────────────
✔ Deploying "shinyappsIO" to "server: shinyapps.io / username: chaitatest"
ℹ Creating application on server...
Error in `POST()`:
! <https://api.shinyapps.io/v1/applications/> failed with HTTP status
  409
Application exists with name: shinyappsIO
Backtrace:
    ▆
 1. └─rsconnect::deployApp(...)
 2.   └─client$createApplication(...)
 3.     └─rsconnect:::POST_JSON(service, authInfo, "/applications/", json)
 4.       └─rsconnect:::POST(...)
 5.         └─rsconnect:::httpRequestWithBody(...)
 6.           └─rsconnect:::handleResponse(httpResponse, error_call = error_call)
 7.             └─rsconnect (local) reportError(json$error)
 8.               └─cli::cli_abort(...)
 9.                 └─rlang::abort(...)
Execution halted

Using the development version from GitHub, the existing app can be redeployed and replaced without any issue.

── Deployment complete ─────────────────────────────────────────────────────────
✔ Successfully deployed to <https://chaitatest.shinyapps.io/SHinyappsIO/>
Deployment completed: https://chaitatest.shinyapps.io/SHinyappsIO/

ChaitaC · 2023-11-08T21:00:59Z

Verified #981
Verified #1007

Collaborator publishing works without any issues. A collaborator can update the content as long as he has an accurate rsconnect dir.

ChaitaC · 2023-11-08T21:14:39Z

⚠️ If a user lacks the necessary permissions to collaborate but possesses the required rsconnect directory, the following error message is displayed:

✅ When a user possesses the appropriate collaborative privileges, the interface appears as follows:

ChaitaC · 2023-11-08T21:16:23Z

@aronatkins the error message 39] in it does not make sense. See for image #1021 (comment)

this is an edge case though

aronatkins · 2023-11-09T14:30:47Z

I have filed #1026 to track the escape codes that are presented by the IDE when the target content has been deleted (or is not visible to the user).

aronatkins added 3 commits October 20, 2023 08:45

shift all deployment target lookups into deploymentTarget ahead of mo…

266789b

…re work

appId collaborator testcases

f528b84

aronatkins requested review from hadley and toph-allen October 23, 2023 19:34

linty

dc9714c

aronatkins commented Oct 23, 2023

View reviewed changes

hadley reviewed Oct 23, 2023

View reviewed changes

aronatkins commented Oct 23, 2023

View reviewed changes

hadley reviewed Oct 24, 2023

View reviewed changes

R/deploymentTarget.R Outdated Show resolved Hide resolved

aronatkins added 6 commits October 24, 2023 08:55

remove debug logging

6b03e9f

getAppByName produces an error, handled when discovering a deployment…

db8b552

… target

deployment target function renaming

14e5f85

deploymentTarget ==> findDeploymentTarget deploymentTargetFromAppId ==> findDeploymentTargetByAppId deploymentTargetFromAppName ==> findDeploymentTargetByAppName updateDeploymentTarget ==> updateDeployment createDeploymentTarget ==> createDeployment

additional findDeploymentTarget explanation

f4b18ac

create findAccountInfo which is an internal accountInfo and allows er…

e97ab2b

…ror context

aronatkins force-pushed the aron-republishing-and-collaborating branch from 32b66af to e97ab2b Compare October 24, 2023 16:00

rename saveDeployment argument from target

429217b

aronatkins commented Oct 24, 2023

View reviewed changes

provide error context to shouldUpdateApp

c34d2b9

aronatkins requested a review from hadley October 24, 2023 16:16

aronatkins added 2 commits October 24, 2023 12:20

lintr: allow longer names

e19cdb2

burned by data.frame factors again

78243f4

hadley approved these changes Oct 24, 2023

View reviewed changes

R/applications.R Outdated Show resolved Hide resolved

R/deploymentTarget.R Show resolved Hide resolved

R/deployments.R Outdated Show resolved Hide resolved

toph-allen approved these changes Oct 24, 2023

View reviewed changes

aronatkins added 3 commits October 25, 2023 08:48

propagate error call to getAppByName

8f8f85e

deployment is named deployment

3399581

comment refinement

86d0d27

This was linked to issues Oct 25, 2023

Unable to publish collaboratively #981

Closed

Collaborator source RMD publishing creates a new app instead of a new bundle #1007

Closed

This was linked to issues Oct 25, 2023

Unable to replace existing shinyapps.io already published #1013

Closed

Behavior when both appname and appid is specified? #1019

Closed

aronatkins merged commit db8c35e into main Oct 25, 2023
10 checks passed

aronatkins deleted the aron-republishing-and-collaborating branch October 25, 2023 13:38

aronatkins added a commit that referenced this pull request Oct 26, 2023

NEWS for #1021

98ccc9a

hadley mentioned this pull request Oct 26, 2023

Published content titles are lowercased #1022

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

republishing and collaborating #1021

republishing and collaborating #1021

aronatkins commented Oct 23, 2023 •

edited

Loading

aronatkins left a comment

hadley Oct 23, 2023

aronatkins Oct 23, 2023

hadley Oct 23, 2023

aronatkins Oct 23, 2023

hadley Oct 24, 2023

aronatkins Oct 23, 2023

aronatkins Oct 23, 2023

aronatkins Oct 24, 2023

hadley left a comment

toph-allen left a comment

ChaitaC commented Nov 1, 2023

ChaitaC commented Nov 8, 2023

ChaitaC commented Nov 8, 2023

ChaitaC commented Nov 8, 2023

aronatkins commented Nov 9, 2023

republishing and collaborating #1021

republishing and collaborating #1021

Conversation

aronatkins commented Oct 23, 2023 • edited Loading

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

Choose a reason for hiding this 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

toph-allen left a comment

Choose a reason for hiding this comment

ChaitaC commented Nov 1, 2023

ChaitaC commented Nov 8, 2023

ChaitaC commented Nov 8, 2023

ChaitaC commented Nov 8, 2023

aronatkins commented Nov 9, 2023

aronatkins commented Oct 23, 2023 •

edited

Loading