-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Save deployment refactoring #702
Conversation
I have made a small behaviour change in the interests of making the user feedback more actionable — if you say you don't want to update the existing app, it now creates a new app. |
Ooops, I should have tested that before implementing — connect requires unique names. |
Re-tested and back to the original behaviour, but with better user feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the workflow when rsconnect discovers a default target that has been deleted on the server? The target
produced by deploymentTarget
has only considered on-disk state, which may be stale?
R/deployApp.R
Outdated
app <- client$getApplication(target$appId) | ||
# Use appId from previous deployment | ||
if (!is.null(target$appId)) { | ||
return(client$getApplication(target$appId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen if the target app ID has been deleted? The old flow would leave app = NULL
and try some other option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it out, and I think if the application id no longer exists, then getApplication()
will error with a 404. That's probably worth considering in more detail (and I'll add a fall back in case getApplication()
does return NULL
in other cases), but I think it's out of scope for this PR.
#Conflicts: # R/deployApp.R
This is a refactoring that shouldn't change behaviour compared to released rsconnect.
I verified expected behaviour using this script:
testdeploy{random number}
index.Rmd
rsconnect::deployApp()
.rsconnect::deployApp()
again.rsconnect::forgetDeployment()
and press enter.rsconnect::deployApp()
again.