-
Couldn't load subscription status.
- 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?
Conversation
prefix from connect() and just use the usual env vars
| ) { | ||
| if (is.null(api_key) || is.na(api_key) || nchar(api_key) == 0) { | ||
| if ( | ||
| missing(token) && is.null(api_key) || is.na(api_key) || nchar(api_key) == 0 |
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 a CONNECT_API_KEY set 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_SERVER environment variable there, so… I think this is a moot point and probably fine.
| @@ -1 +1 @@ | |||
| test_conn_1 <- connect(prefix = "TEST_1") | |||
| client <- 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 took the opportunity to rename all of these in the integration tests to be client.
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 looks good! I think it's unlikely that customers used this functionality, but do you think we should list it in NEWS.md as a breaking change? I'll let you use your judgment on that; this can merge whatever your decision there.
a347014 to
5b9feb5
Compare
Intent
Remove prefix argument to
connect()and just useCONNECT_API_KEYandCONNECT_SERVERenv vars like everything else does. Also moves some validation tests forconnect()to the unit tests, and tamp down on some of the spurious messaging that was confusing when we were looking at the integration tests in the last PR.Fixes #476
Approach
Delete stuff, move stuff, read code, run tests.
Checklist
NEWS.md(referencing the connected issue if necessary)?devtools::document()?