-
Notifications
You must be signed in to change notification settings - Fork 31
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
More SSL/container fixes. #276
Conversation
* In order to disable SSL by default, the SSL variable has to be set to 0 all other values enable SSL. * Make the notebooks consistent with this and print the proper URL depending on whether we are using SSL. * Copy over the opensearch host auto-guessing logic from default-prep-script.ipynb
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.
Seems OK, but I have some questions.
@@ -10,5 +10,6 @@ UI_PORT=3000 | |||
OPENSEARCH_PORT=9200 | |||
RAY_CONSOLE_PORT=8265 | |||
JUPYTER_PORT=8888 | |||
SSL=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.
What is the top-level .env file and what does it do? Seems like a dangerous default setting for SSL.
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.
It sets the default environment when you run a docker compose. If we don't do this, then people will get https if they don't do it manually, and Jon said we had to http. I'm assuming other stuff won't read this.
@@ -31,7 +31,7 @@ def doFile(name, dir, ent): | |||
top = ast.parse(fp.read()) | |||
|
|||
ary = [] | |||
base = ent[ : -3] | |||
base = ent[:-3] |
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 always disagree with black on this one. Colon is a binary operator. There should be spaces around it. And also, I didn't think this file was under auto/enforced formatting. GitHub doesn't seem to think it is.
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.
pre-commit does this:
poetry run pre-commit run --all-files
fp.write(f".. autoclass:: {sym}\n" + | ||
" :members:\n" + | ||
" :show-inheritance:\n") | ||
fp.write(f".. autoclass:: {sym}\n" + " :members:\n" + " :show-inheritance:\n") |
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.
Does black really think this is better? It could at least concatenate the strings.
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.
It's what it does.
notebooks/jupyter_dev_example.ipynb
Outdated
@@ -252,8 +252,16 @@ | |||
"source": [ | |||
"index = \"local_development_example_index_withentity\" # You can change this to something else if you'd like\n", | |||
"\n", | |||
"os_client_args = {\n", | |||
" \"hosts\": [{\"host\": \"localhost\", \"port\": 9200}],\n", | |||
"if os.path.exists(\"/app/work/bind_dir\"):\n", |
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 think the canonical way to check is to look for the file /.dockerenv but any of these methods can be fooled.
@@ -319,7 +329,7 @@ | |||
"name": "python", | |||
"nbconvert_exporter": "python", | |||
"pygments_lexer": "ipython3", | |||
"version": "3.11.6" | |||
"version": "3.11.2" |
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.
Is this setting a minimum version?
In order to disable SSL by default, the SSL variable has to be set to 0 all other values enable
SSL.
Make the notebooks consistent with this and print the proper URL depending on whether we are
using SSL.
Copy over the opensearch host auto-guessing logic from default-prep-script.ipynb