-
Notifications
You must be signed in to change notification settings - Fork 67
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
RHOAIENG-16604: feat(RStudio): grab proxy-related env vars from container env context and set them in R default environment #797
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @shalberd. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
e57eb03
to
6aa943b
Compare
/retest |
Hi @jiridanek unsure why the tests fail on setup of the cluster, not execution, it seems. Same for the other PR #798 |
Hi @shalberd, thank you for this contribution. The change seems fine, I put one comment there. The failed tests are probably because of the env instability of the big image building. If you want to check the final image that is based on your changes, you can get the image from https://quay.io/repository/opendatahub/workbench-images?tab=tags&tag=latest and use filter for |
f68eb64
to
fbcec28
Compare
…, setting them in R global env file. Also changed env var search to beginning-of-line regex format for all env vars. Signed-off-by: shalberd <[email protected]>
fbcec28
to
a1b1789
Compare
@shalberd: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@jiridanek @jstourac tested with built image situation before: pre-image Change with env variables Terminal Container
Terminal in R Studio GUI:
Container proxy-related env variables not available in R Studio and thus e.g. version Control / Git R Project pull times out
Image PR-797: Terminal Container
all previous KUBERNETES env variables in global file, as before, plus new the container proxy-related env variables:
Terminal in R Studio GUI
same as before PR
now, proxy-related env vars are available in R Studio. Creating a new project with version control Gitlab / Github Url not listed in no_proxy no longer leads to timeout. Checkout successful. |
@shalberd: shalberd unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
:-) |
OpenShift CI is having a bad day, looks like. |
that, and I was not able to override with |
there's enough time to override when the pr is about to go in; openshift-ci likes to retest just before merge, so overriding in advance turns to be a waste of time, usually |
https://issues.redhat.com/browse/RHOAIENG-16604
@guimou Needed so that R proxy settings do not need to be done at user-home level, instead globally, if the information is already present in notebook container env vars.
https://support.posit.co/hc/en-us/articles/200488488-Configuring-R-to-Use-an-HTTP-or-HTTPS-Proxy
Description
added one more line to startup script, similar to the already-existing KUBERNETES_ env variables.
Only difference in R Studio is, key needs to be lowercased.
How Has This Been Tested?
command-line tested, tested in ~/.Renviron
Created a new project in R studio with version control.
Url of Git used required use of proxy cause it is not directly accessible.
Worked fine.
If this PR can create a PR- labeled notebook image R studio, I can test, too.
Merge criteria: