Made the www-frame-origin for rserver environment-configurable

[rickmcgeer]

This is the proposed solution to: #147. Configuration by traitlets, as proposed in the issue, is inappropriate since the command to set up the rserver and rsession is invoked when the package is loaded, so any configuration has to be present in the environment when the package is loaded. Adding configuration by environment variables is consistent with the existing package architecture, and in fact it was done (hardcoded) in setup_rserver._get_timeout and setup_rsession._get_timeout.
Changes made:

1. New top-level method get_env_value(env_var_name) to get the value for the given environment variable. Defaults are provided for configurable environment variables.
2. Added RSERVER_FRAME_ORIGIN environment variable to make www-frame-origin environment-configurable. The default value is the same as the current hard-coded value.
3. Added RSERVER_TIMEOUT environment variable and replaced setup_rserver._get_timeout() with get_env_value('RSERVER_TIMEOUT'). Default is the same as the current hardcoded default (15)
4. Added RSESSION_TIMEOUT environment variable and replaced setup_rsession._get_timeout() with get_env_value('RSESSION_TIMEOUT'). Default is the same as the current hardcoded default (15)
5. Replaced user = os.environ.get('NB_USER', getpass.getuser()) with get_env_value('NB_USER'). Default for NB_USER is getpass.get_user()
6. Updated README.md with information on using configuration variables.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[ryanlovett]

Preferring environment variables instead of traitlets is totally fine. It also makes sense given that we're currently not putting in traitlets directly. Also, thanks for documenting the environment variables in the README!

I think I prefer to have the value retrieval happen in separate functions, akin to _get_timeout(), rather than a single get_env_value function for all variables. Perhaps this is just a style choice, although using separate functions would allow us to one day pivot to using traitlets' default decorator.

I'm wondering whether the environment variable names should indicate the jupyter-rsession-proxy context in some way. e.g. JUPYTER_RSESSION_PROXY_FOO, instead of just FOO. This helps to clarify that the variables are not consumed by the actual rsession or rserver binaries. The downside is that variables are kind of long:

JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN
JUPYTER_RSESSION_PROXY_RSERVER_TIMEOUT

We'd also need to communicate this if we were to change an existing env var.

This issue inspired me to create #149.

Lastly, it looks like you accidentally included jupyter_rsession_proxy.egg-info and its contents.

If what I wrote isn't clear, or if you'd just prefer, I think I can modify the PR to reflect the above.

[rickmcgeer]

@ryanlovett The only reason I would do this is to learn how, but I think I already know and you doing it saves us a loop. Sold, with (as always) thanks!

1. The (minor) case for get_env_value is that adding a new environment variable is a little easier than a dedicated function (just add an entry to a dictionary). But what's much more important is to maintain consistency with the current code base, and you have a much better handle on that than I do. Also, as you point out, using a dedicated function abstracts away how these values are getting set, and so traitlets in the future will be more seamless. And the it's-a-little-easier argument is very weak; after all, we won't have all that many environment variables, so the effect on the code by using dedicated functions is very minor. So, please, use your best judgment here -- I am happy either way. Can I propose _get_rserver_frame_origin() or similar for the dedicated function?
2. JUPYTER_RSESSION_PROXY_FOO is much better than FOO. Sorry, I should have thought of it...please make that change.
3. I'm sorry about the egg-info. What happened was (I'm explaining so you can tell me how not to screw this up in future!) is that I needed to clone and install the package and hardcode the www-frame-origin=any flag, and to make this seamless with pip git+https I cloned the egg-info. I then branched off that fork. I'm sure I could have done that better.

Please go ahead and make all those changes.

Thanks again for everything
Rick

[ryanlovett]

@rickmcgeer Is the setting to allow edits from maintainers enabled in your fork's PR? I don't think I can edit the PR otherwise. (I've tried and failed, but I could be doing something wrong.) Alternatively, I can make a new PR and pull in your changes.

[rickmcgeer]

@ryanlovett my apologies; I don't know. The doc you referenced says "On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.". I've been unable to find the screen with the field "Allow edits from maintainers" (as with many infrastructures, Github has a security/permissions system designed by Franz Kafka and administered by the DMV and the City of Dis as told by Niven and Pournelle in Inferno). Anyway, I made you an admin of our repo, so I hope that's enough. Sorry for the trouble,...

[ryanlovett]

@rickmcgeer :) "It was on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying 'Beware of the Leopard.'"

Its no trouble, thanks! I think this should be sufficient.

[ryanlovett]

With this PR, I've confirmed that setting JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN=any causes --www-frame-origin=any to be passed to rserver.

I didn't change the other env vars as part of this PR to avoid breaking things for others. That can be done separately in an announced change.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[ryanlovett]

Thanks @rickmcgeer !

[rickmcgeer]

Thanks @ryanlovett ! It was a real pleasure to work with you and I hope to do so again soon.

[ryanlovett]

Absolutely @rickmcgeer ! We'd be happy to receive more code contributions. :)

[rickmcgeer]

@ryanlovett Happy to contribute! On that note, should we enhance the Shiny proxy the same way?
And the next time we do this (for the Shiny proxy or another piece of code), I'd like to seek your guidance on the right way to fork the repo so we don't have the permissions issues that we had to deal with this time. If I'd done it right that wouldn't have happened.