Simply `deploymentTarget()` logic #666

hadley · 2023-02-13T17:21:32Z

This PR went a bit out of control, but it does two main things:

Introduces a new findAccounts() function that has extensive messaging to help you when an account isn't found (this fixes a pet peeve of mine). This is now used everywhere that resolveAccount() was previously used.
Uses findAccounts() and some thought to simplify deploymentTargets(); I don't think the logic is completely the same but I think it should be close and it's now much much easier to reason about. I've updated the documentation to describe this new logic, and in general how updating works.

Sorry this is so big, and I'm happy to discuss in real-time if you think that would be useful.

* Eliminates need for `resolveAccount()` in most places * Delete now unused `stopWith*()` functions * Use shared documentation

hadley · 2023-02-13T17:25:43Z

R/deploymentTarget.R

@@ -214,25 +71,3 @@ createDeploymentTarget <- function(appPath,
    server = server
  )
 }
-
-findAppId <- function(appPath, appName, account, server, username) {


This is no longer needed because either we are deploying for the first time, in which case the appId will be NULL, or we're deploying for the second time, in which case we take the appId from the previous deployment.

hadley · 2023-02-13T17:44:27Z

R/deploymentTarget.R

-        appDeployments$server
-      )
-    }
+  appDeployments <- deployments(


This is a change in behaviour — now we consider only deployment which match the suppled, appName, account and server. Previously (I think) supplying either server or account always generated a new deployment.

#Conflicts: # DESCRIPTION

hadley · 2023-02-13T18:56:40Z

R/deploymentTarget.R

-
-
-  # if appTitle specified but not appName, generate name from title
-  if (is.null(appName) && !is.null(appTitle) && nzchar(appTitle)) {


This is now only done on first deploy as documented. And I'm pretty sure the previous behaviour was incorrect as it meant that setting a new appTitle would generally result in publishing a new app, rather than updating an existing app.

Could you check what values are provided by the IDE? Its conventions have changed over time. In particular, what happens when going through the "Other Destination" workflow? Is that different from a "republish" workflow?

I don't recall what of name/title is given by the IDE. It would be good to capture that expectation.

We'll probably want to retest a handful of "new deployment" versus "overwrite deployment" workflows. CC @ChaitaC

Here's what I see in the "Other destination" dialog — it like the UI allows you to choose the appTitle, which appears to be auto generated from the doc name:

It's not clear what it users for the appName. I'll need to do some more exploration.

The republish dialog only allows me to change which files are published.

Note that the id also uses the output of applications() to find the appId based on the appName, so that even if deployments() is empty, it'll still attempt to update an existing bundle.

I've made an issue #669 to track logging more information so it's easier to verify this. I'll do that in a separate PR because this one is already big.

hadley · 2023-02-13T19:15:09Z

R/deployments.R

-  # TODO(HW): move this logic into deploymentTarget
-  # if there's no new title specified, load the existing deployment record, if
-  # any, to preserve the old title
-  if (is.null(title) || is.na(title) || length(title) == 0 ||


saveDeployment() is only called from deployApp() where title is the appTitle returned by deploymentTitle(), so I moved the logic there.

#Conflicts: # NEWS.md

aronatkins · 2023-02-13T21:58:35Z

R/tasks.R

-#'   then this parameter can be omitted.
-#' @param server Server name. Required only if you use the same account name on
-#'   multiple servers (see [servers()])
+#' @inheritParams deployApp


The roxygen documentation for inheritParams (https://roxygen2.r-lib.org/articles/reuse.html) should contain additional examples showing which parameter documentation is inherited (only the overlapping arguments), and how one would combine inheritance with replacement / additional documentation.

Doc issue at r-lib/roxygen2#1465, and a better long form explanation at https://r-pkgs.org/man.html#inheriting-arguments

R/deployApp.R

aronatkins · 2023-02-13T22:06:54Z

R/deploymentTarget.R

-
-
-  # if appTitle specified but not appName, generate name from title
-  if (is.null(appName) && !is.null(appTitle) && nzchar(appTitle)) {


Could you check what values are provided by the IDE? Its conventions have changed over time. In particular, what happens when going through the "Other Destination" workflow? Is that different from a "republish" workflow?

I don't recall what of name/title is given by the IDE. It would be good to capture that expectation.

We'll probably want to retest a handful of "new deployment" versus "overwrite deployment" workflows. CC @ChaitaC

aronatkins · 2023-02-13T22:09:10Z

R/import-standalone-obj-type.R

@@ -0,0 +1,348 @@
+# Standalone file: do not edit by hand


One of your other PRs is pulling in rlang; does this file go away with an rlang dependency?

We haven't yet standardised this interface, so we're currently vendoring it into every package where we use it. I think it's worth the cost, because it makes input validation so much more pleasant. At some point once we provide an exported interface, we'll be able to remove from this and other packages.

toph-allen

Upon a read-through, I like these changes. We should probably make sure we put these changes through QA, but if there's more coming down the pike, maybe it makes sense to bundle them and have @ChaitaC look at them together.

R/acccount-find.R

toph-allen · 2023-02-13T22:59:06Z

R/accounts.R

-    stopWithMultipleAccounts(name)
-  if (length(configFile) == 0 || !file.exists(configFile))
-    stop(missingAccountErrorMessage(name))
+  fullAccount <- findAccount(name, server)


Wrapping all that logic up into a single function makes me v happy.

Me too! It also always bugged me that I had to supply both name and server even though all the servers I publish to have the same username.

hadley added 10 commits February 13, 2023 10:34

Import rlang

917d4a9

Bring in type checks from rlang

7469bb2

Implement and test new findAccount() function

cfcfbfd

Use accountFind() in deployment targets

67121fe

Improve error for multiple deployments

2367844

Simplify deployment logic

dfe9a46

Test that can override title

d81ceeb

Also filter bsaed on account + server

183e733

Use findAccount() in account functions

d949ac0

* Eliminates need for `resolveAccount()` in most places * Delete now unused `stopWith*()` functions * Use shared documentation

Needs cli

b8d91f6

hadley commented Feb 13, 2023

View reviewed changes

hadley added 4 commits February 13, 2023 12:27

Merge commit 'aecbd89d2f7998750a4f79ed480a11c67fc03d84'

37562f6

#Conflicts: # DESCRIPTION

Bullet about validation

1372cfc

Document update logic

7eb3cd3

Only generate appName on first deploy

2db5054

hadley commented Feb 13, 2023

View reviewed changes

hadley added 2 commits February 13, 2023 12:58

appName can also resolve ambiguity

9c9c919

Move default title logic out of saveDeployment()

094b223

hadley commented Feb 13, 2023

View reviewed changes

hadley added 2 commits February 13, 2023 13:22

Tweak saveDeployment() arguments to simplify test usage

dfff0b4

Merge commit '3988e6de2b9d6825932a43c6fe8e0260bda97e1d'

b95fcc0

#Conflicts: # NEWS.md

hadley requested review from toph-allen and aronatkins February 13, 2023 19:25

hadley marked this pull request as ready for review February 13, 2023 19:25

aronatkins approved these changes Feb 13, 2023

View reviewed changes

More info on where to find appId

78ac5af

toph-allen approved these changes Feb 13, 2023

View reviewed changes

hadley added 3 commits February 14, 2023 07:41

account has two c's

7662af3

Fix typo

7f3178f

Update other snapshot

92bcfae

hadley merged commit 2dec764 into main Feb 14, 2023

hadley deleted the deploymentTarget-errors branch February 14, 2023 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simply `deploymentTarget()` logic #666

Simply `deploymentTarget()` logic #666

hadley commented Feb 13, 2023 •

edited

Loading

hadley Feb 13, 2023

hadley Feb 13, 2023

hadley Feb 13, 2023

aronatkins Feb 13, 2023

hadley Feb 13, 2023

hadley Feb 13, 2023

hadley Feb 14, 2023

hadley Feb 13, 2023 •

edited

Loading

aronatkins Feb 13, 2023

hadley Feb 14, 2023

aronatkins Feb 13, 2023

aronatkins Feb 13, 2023

hadley Feb 13, 2023

toph-allen left a comment •

edited

Loading

toph-allen Feb 13, 2023

hadley Feb 14, 2023



		# if appTitle specified but not appName, generate name from title
		if (is.null(appName) && !is.null(appTitle) && nzchar(appTitle)) {

Simply deploymentTarget() logic #666

Simply deploymentTarget() logic #666

Conversation

hadley commented Feb 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley Feb 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toph-allen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simply `deploymentTarget()` logic #666

Simply `deploymentTarget()` logic #666

hadley commented Feb 13, 2023 •

edited

Loading

hadley Feb 13, 2023 •

edited

Loading

toph-allen left a comment •

edited

Loading