Reduce the usage of appPath #696
Conversation
* I think the hooks should always get a directory, not sometimes a path. * It's clear that `deploymentTarget` should get the path where the deployments are recorded
|
I think this suggests that
So I reviewed the code from v0.8.29 (https://github.com/rstudio/rsconnect/blob/v0.8.29/R/deployApp.R) to make sure I hadn't accidentally refactored out some other use case, but it doesn't look like it. And looking at the commit where it was added shows that If I deprecate it, also need to adjust |
| @@ -237,7 +237,7 @@ deployApp <- function(appDir = getwd(), | |||
| } | |||
|
|
|||
| # invoke pre-deploy hook if we have one | |||
| runDeploymentHook(appPath, "rsconnect.pre.deploy", verbose = verbose) | |||
| runDeploymentHook(appDir, "rsconnect.pre.deploy", verbose = verbose) | |||
There was a problem hiding this comment.
This is a functional change to package contract, correct? Given a target Rmd in a directory containing lots of Rmd, how will the hook know what you're about to deploy?
This work initially landed with #283 and #295, but without a specific motivating example.
Additionally, I think the IDE deploy pane transfers many (most? all?) R options and environment variables from the R session to the deploy pane (without using these hooks).
I did find one issue referencing this hook:
and a reference from thematic!!:
Unfortunately, I don't know of many in-the-wild uses of this feature that might care about the file-path vs. directory change. At the very least, we should NEWS this adjustment and mark it as a potentially breaking change.
There was a problem hiding this comment.
Yeah, this is a change, and the documentation for the hooks is murky:
\item{\code{rsconnect.pre.deploy}}{A function to run prior to deploying content; it receives as an argument the path to the content that's about to be deployed.}
What does "content about to be deployed" mean? (especially if you've used appFiles or friends).
My guess is that this won't affect any code in the wild, but I'll add a news bullet.
FWIW I think the thematic usage would break when appPath is not a directory.
|
Agreed that |
| @@ -247,7 +247,7 @@ deployApp <- function(appDir = getwd(), | |||
| } | |||
|
|
|||
| # determine the deployment target and target account info | |||
| target <- deploymentTarget(appPath, appName, appTitle, appId, account, server) | |||
| target <- deploymentTarget(recordDir, appName, appTitle, appId, account, server) | |||
There was a problem hiding this comment.
Internally, deploymentTarget calls this argument appPath; should it be renamed?
There was a problem hiding this comment.
I've renamed it to recordPath, in anticipation of renaming recordDir to recordPath in a future PR.
| @@ -18,11 +18,10 @@ deploymentTarget <- function(appPath = ".", | |||
| if (nrow(appDeployments) == 0) { | |||
| fullAccount <- findAccount(account, server) | |||
| if (is.null(appName)) { | |||
| appName <- generateAppName(appTitle, appPath, account, unique = FALSE) | |||
| appName <- generateAppName(appTitle, recordPath, account, unique = FALSE) | |||
There was a problem hiding this comment.
This reinforces that the recordPath really should be original source of the content.
I think the hooks should always get a directory, not sometimes a path.
It's clear that
deploymentTargetshould get the path where the deployments are recorded@aronatkins this is part of a very gradual unpicking of the complicated
appPath/recordDirlogic indeployApp().