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

add support for variable host and port as suggested in #210 #211

Merged
merged 2 commits into from May 5, 2015

Conversation

Projects
None yet
2 participants
@cboettig
Contributor

cboettig commented Mar 18, 2015

A heck, here's a PR to implement this anyway, that just makes host and port into optional function arguments. Let me know if you'd rather it were done differently. Many thanks

@cboettig

This comment has been minimized.

Contributor

cboettig commented Apr 28, 2015

@hadley ping. I know httr is still in the queue behind lots of other good stuff; but this is a quick fix to an issue that bites me regularly. (I'd just use my branch but seems weird to tell users to do that)

@hadley

This comment has been minimized.

Member

hadley commented Apr 28, 2015

I'll add httr to my list of packages that need updates (it's currently behind devtools and tidyr)

@hadley

This comment has been minimized.

Member

hadley commented May 4, 2015

Could you add a bullet point to NEWS please?

@@ -7,10 +7,13 @@
#' \code{\link{oauth_app}}
#' @param permission optional, a string of permissions to ask for.
#' @param is_interactive Is the current environment interactive?
#' @param host ip address for the listener

This comment has been minimized.

@hadley

hadley May 4, 2015

Member

@inheritParams ouath_listener would be a bit DRYer

This comment has been minimized.

@cboettig

cboettig May 4, 2015

Contributor

Thanks for these, will do

On Mon, May 4, 2015, 11:41 AM Hadley Wickham notifications@github.com
wrote:

In R/oauth-init.R
#211 (comment):

@@ -7,10 +7,13 @@
#' \code{\link{oauth_app}}
#' @param permission optional, a string of permissions to ask for.
#' @param is_interactive Is the current environment interactive?
+#' @param host ip address for the listener

@inheritParams ouath_listener would be a bit DRYer


Reply to this email directly or view it on GitHub
https://github.com/hadley/httr/pull/211/files#r29610720.

@hadley hadley merged commit f2a2e9a into r-lib:master May 5, 2015

@hadley

This comment has been minimized.

Member

hadley commented May 5, 2015

Thanks!

@cboettig

This comment has been minimized.

Contributor

cboettig commented May 15, 2015

@hadley Now I realize that it probably would have been better to set the default of the function argument to something like getOption("httr_localhost") since most third-party R packages define their own auth wrappers around httr and aren't exposing this option either.

To my thinking, a better implementation would actually check the environmental variable for the default, since I could then set something like ENV LOCALHOST 0.0.0.0 inside the container and everything would work with no additional intervention. But I'm not sure if calling Sys.getenv("LOCALHOST") is a good idea. Thoughts?

@cboettig

This comment has been minimized.

Contributor

cboettig commented May 15, 2015

@hadley More problematically, I'm worried that this PR might have introduced a bug to oauth as well. I currently get the following error with traceback:

 httr::oauth2.0_token(dropbox, dropbox_app)
Error in makeTcpServer(host, port, appWrapper$onHeaders, appWrapper$onBodyData,  : 
  object 'host' not found

Enter a frame number, or 0 to exit   

1: httr::oauth2.0_token(dropbox, dropbox_app, host = "0.0.0.0")
2: Token2.0$new(app = app, endpoint = endpoint, params = params, cache_path = 
3: public_bind_env$initialize(...)
4: self$init_credentials()
5: init_oauth2.0(self$endpoint, self$app, scope = self$params$scope, type = se
6: oauth_listener(authorize_url, is_interactive, host, port)
7: httpuv::startServer(host, port, list(call = listen))
8: makeTcpServer(host, port, appWrapper$onHeaders, appWrapper$onBodyData, appW

If I go into the environment at 6, oauth_listener it says:

Selection: 6
Browse[4]> ls()
[1] "host"           "info"           "is_interactive" "listen"        
[5] "port"           "request_url"   
Browse[4]> host
Error during wrapup: promise already under evaluation: recursive default argument reference or earlier problems?

even though at 1 the value host is correctly identified. I'm not sure if something has changed since the PR, but at that time I could confirm that this set the host correctly without error. Not sure what's up. maybe I should re-open or open a new issue?

@hadley

This comment has been minimized.

Member

hadley commented May 18, 2015

An env var seems ok to me, but it should be something like HTTR_LOCALHOST. I'd recommend making a little function to wrap it up.

I can't replicate the bug you're seeing

@cboettig

This comment has been minimized.

Contributor

cboettig commented May 18, 2015

@hadley It's definitely a bug. Looks like I should have edited the R6 Token class to include the host variable and define self$host <- host, e.g. https://github.com/hadley/httr/blob/master/R/oauth-token.r#L40-L45

My apologies, I completely overlooked what was going on in the R6 classes when working out this pull request but @richfitz set me straight.

install_github("cboettig/rdrop2")
rdrop2::drop_auth(host="0.0.0.0")

should replicate this issue(?)

It looks like I need to modify https://github.com/cboettig/httr/blob/master/R/oauth-token.r#L40-73 to include host and self as part of the definition, but I'm hesitant to go hacking fundamental class structures without really knowing what I'm doing...

@hadley

This comment has been minimized.

Member

hadley commented May 19, 2015

I think you'll need to add a listener field that stores the host and port (and anything else that might come up in the future)

hadley added a commit that referenced this pull request May 22, 2015

Merge pull request #235 from cboettig/master
Work around listener errors introduced in #211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment