Skip to content
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.

Sign up for GitHub

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

Fix checking and logging of envVars in deployApp() #994

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix checking and logging of envVars in deployApp() #994

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6d4ae9d
fix(deployApp): Don't reveal envvar value in logs
gadenbuie Sep 8, 2023
523f1e5
fix(deployApp): Fixes an off-by-one error when checking `envVars`
gadenbuie Sep 8, 2023
f3dac5b
chore: Check `client` directly to know if it supports `setEnvVars`
gadenbuie Sep 8, 2023
28531f1
Revert "fix(deployApp): Don't reveal envvar value in logs"
gadenbuie Sep 9, 2023
0c9bfbf
docs(deployApp): Add advice for setting env vars
gadenbuie Sep 9, 2023
8a74325
feat(deployApp): Throw if `envVars` is a named vector
gadenbuie Sep 9, 2023
4a88330
Update deployApp.R
gadenbuie Sep 9, 2023
dfe66d5
Update deployApp.R
gadenbuie Sep 9, 2023
f1227e4
docs(deployApp): Recommend Renviron or {keyring} for `envVars`
gadenbuie Sep 9, 2023
dd12337
tests: Update snapshots
gadenbuie Sep 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions R/deployApp.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ deployApp <- function(appDir = getwd(),
stop("Posit Connect does not support deploying without uploading. ",
"Specify upload=TRUE to upload and re-deploy your application.")
}
if (!isConnectServer(target$server) && length(envVars) > 1) {
if (!isConnectServer(target$server) && length(envVars) > 0) {
cli::cli_abort("{.arg envVars} only supported for Posit Connect servers")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than checking here by enforcing that target$server points to a Connect server, you could instead check just a little bit further down after creating client. In that case, you could use a block like the following, and check that client implements the setEnvVars() psuedo-method.

  client <- clientForAccount(accountDetails)
  if (length(envVars) > 0 && !"setEnvVars" %in% names(client)) {
    cli::cli_abort("{target$server} does not support setting {.arg envVars}")
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea. Want to do it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, done in f3dac5b


Expand Down Expand Up @@ -432,7 +432,7 @@ deployApp <- function(appDir = getwd(),
taskComplete(quiet, "Visibility updated")
}
if (length(target$envVars) > 0) {
taskStart(quiet, "Updating environment variables {envVars}...")
taskStart(quiet, "Updating {length(envVars)} environment variable{?s}: {.field {names(envVars)}}...")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already just the names of the env vars?

Copy link
Member Author

@gadenbuie gadenbuie Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had a hard time following the flow for that property. What happened for me -- before I applied the other fix -- was that I tried to deploy an app with one env var to shinyapps.io. Something like envVars = c("FLAG" = "true"). This made it past the first check and failed here (because the shinyapps client doesn't implement $setEnvVars()) at which point this call was printing environment variables true.

I made this change first and then realized that the earlier check was failing, and in my probably semi-broken state envVars was a named vector. This may have been the result of a local rsconnect deploy record with a value that shouldn't have been written?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe we need some earlier validation that envVars is an unnamed character vector?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, I read the documentation more carefully and realized that the environment variables should be set in the current session and the user should provide just the names, as in envVars = c("FLAG", "OTHER_FLAG").

I think my confusion stemmed from being familiar with connectapi, where there's an interface for setting environment variables during deployment that uses the FLAG = "value" pattern.

I updated the docs to give a little guidance about how those values should be set, and I added a check that the vector provided to envVars is unnamed. This feels a little aggressive but I think it's worth it, given the potential risk of leaking a credential if a user misunderstands how envVars should be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it's not clear, the interface looks like this is to prevent you from typing the values of env vars in executed code since that will be captured in .Rhistory.

client$setEnvVars(application$guid, target$envVars)
taskComplete(quiet, "Environment variables updated")
}
Expand Down