-
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
don't restrict shinyapps.io from previous deployment check in deploymentTarget #933
Conversation
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.
Once you prove that this works, NEWS and test, please.
… for 932 bugfix" This reverts commit 78ddd1d.
app_dir <- withr::local_tempdir() | ||
target <- deploymentTarget(app_dir, appName = "my_app") | ||
expect_equal(target$appId, 123) | ||
for (server in c("example.com", "shinyapps.io")) { |
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 think this is best done with a helper function and two invocations of that test function -- that will help us understand which condition is failing. Something like:
confirm_existing_application_use <- function(server) {
# innards of loop
}
test_that("can find existing application on server & use it", {
confirm_existing_application_use("example.com")
})
test_that("can find existing application on shinyapps.io & use it", {
confirm_existing_application_use("shinyapps.io")
})
@hadley - do you have different recommendations?
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.
This tests out properly for me. Running interactively I am prompted whether to redeploy to the application that was found, running noninteractively it assumes that's what I want, which is consistent with prior behavior.
No description provided.