- 
                Notifications
    You must be signed in to change notification settings 
- Fork 26
          refactor: remove prefix from connect() and just use the usual env vars
          #477
        
          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
base: main
Are you sure you want to change the base?
Changes from all commits
41bc59c
              2e50b82
              925a558
              f141e24
              5b9feb5
              8b129cc
              9971355
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1 +1 @@ | ||
| test_conn_1 <- connect(prefix = "TEST_1") | ||
| client <- connect() | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took the opportunity to rename all of these in the integration tests to be  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1,78 +1,12 @@ | ||
| test_that("connect works", { | ||
| conn <- connect( | ||
| server = Sys.getenv("TEST_1_SERVER"), | ||
| api_key = Sys.getenv("TEST_1_API_KEY") | ||
| ) | ||
| expect_true(validate_R6_class(conn, "Connect")) | ||
| }) | ||
|  | ||
| test_that("connect works with prefix only", { | ||
| conn <- connect(prefix = "TEST_1") | ||
| expect_true(validate_R6_class(conn, "Connect")) | ||
| }) | ||
|  | ||
| test_that("connect fails for nonexistent server", { | ||
| expect_error({ | ||
| connect(server = "does-not-exist.rstudio.com", api_key = "bogus") | ||
| }) | ||
| # This whole suite assumes CONNECT_SERVER and CONNECT_API_KEY env vars are set | ||
| test_that("connect() works", { | ||
| expect_true(validate_R6_class(connect(), "Connect")) | ||
| }) | ||
|  | ||
| test_that("connect fails for good server, bad api key", { | ||
| expect_error({ | ||
| connect( | ||
| server = Sys.getenv("TEST_1_SERVER"), | ||
| api_key = "bogus" | ||
| ) | ||
| }) | ||
| }) | ||
|  | ||
| test_that("error if API key is empty", { | ||
| expect_error( | ||
| connect(server = Sys.getenv("TEST_1_SERVER"), api_key = ""), | ||
| "provide a valid API key" | ||
| ) | ||
|  | ||
| expect_error( | ||
| connect(server = Sys.getenv("TEST_1_SERVER"), api_key = NA_character_), | ||
| "provide a valid API key" | ||
| ) | ||
|  | ||
| expect_error( | ||
| connect(server = Sys.getenv("TEST_1_SERVER"), api_key = NULL), | ||
| "provide a valid API key" | ||
| ) | ||
| }) | ||
|  | ||
| test_that(".check_is_fatal toggle works", { | ||
| expect_error( | ||
| connect(server = Sys.getenv("TEST_1_SERVER"), api_key = ""), | ||
| "provide a valid API key" | ||
| ) | ||
|  | ||
| rsc <- connect( | ||
| server = Sys.getenv("TEST_1_SERVER"), | ||
| api_key = "", | ||
| .check_is_fatal = FALSE | ||
| ) | ||
| expect_true( | ||
| validate_R6_class(rsc, "Connect") | ||
| ) | ||
|  | ||
| expect_error( | ||
| suppressMessages(connect( | ||
| server = "http://fake-value.example.com", | ||
| api_key = "fake-value" | ||
| )), | ||
| "Could not resolve host" | ||
| ) | ||
|  | ||
| # TODO: suppressing the message in the tryCatch handler does not work...? | ||
| rsc1 <- suppressMessages(connect( | ||
| server = "http://fake-value.example.com", | ||
| api_key = "fake-value", | ||
| .check_is_fatal = FALSE | ||
| )) | ||
| expect_true( | ||
| validate_R6_class(rsc1, "Connect") | ||
| ) | ||
| }) | 
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 added this
missing(token)bit because it seemed odd that we would validate the API key if instead we were going to use the token. Seemed like we were relying on the fact that there happens to be aCONNECT_API_KEYset on Connect always. But maybe I misunderstood: do you need an API key in order to exchange the token?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.
Hmm — I think this is the case, I think you need the API key to exchange the token (see that a client is created on line 1015).
However, token exchange only ever happens on Connect and we always have the
CONNECT_SERVERenvironment variable there, so… I think this is a moot point and probably fine.