-
Notifications
You must be signed in to change notification settings - Fork 30
Update localhost refs #959
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: master
Are you sure you want to change the base?
Conversation
This reverts commit 82298fe.
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.
Only support setting db-uri using CLI flag
analysis/analysis/analysis.py
Outdated
return | ||
|
||
cp = ConfigParser() | ||
with conf_file.open("r") as f: |
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 would suggest dropping all this config file handling stuff and only allow setting db-uri via the CLI flag --db-uri
.
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.
The reason is that we are potentially adding some extra code paths that can be tricky to debug if something is not working as intended.
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.
How do we set up the flag --db-uri
when deployed? I originally used the conf file because it's where we specify configurations in Ansible
Update all clickhouse localhost refs in backend to use a configuration parameter instead.
It's set up such that if no such configuration is provided, it defaults to the old behaviour of having localhost hardcoded
The following config file requires an update:
analysis.conf
Closes #954
Related to ooni/devops#242