-
Notifications
You must be signed in to change notification settings - Fork 2
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
Migration of autocomplete to new DB schema #375
Conversation
✔️ Deploy Preview for phenopolis-dev canceled. 🔨 Explore the source changes: 5a570ab 🔍 Inspect the deploy log: https://app.netlify.com/sites/phenopolis-dev/deploys/618118303923320007fa1b40 |
README.md
Outdated
@@ -20,6 +20,7 @@ AWS_SECRET_ACCESS_KEY=.... | |||
AWS_ACCESS_KEY_ID=.... | |||
|
|||
NETLIFY_AUTH_TOKEN=.... | |||
NETLIFY_SITE_ID=.... |
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 we should remove both of these from the README...
This is only needed if you want to do fancy stuff with netlify dev
- e.g. use the --live
flag (which publishes to a live sharable URL instead of localhost)...
As such - it's complicating things unnecessarily
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.
OK, but where I should I put them:
NETLIFY_AUTH_TOKEN=....
NETLIFY_SITE_ID=....
in views/__init__.py
, right? If so I can do that in this PR.
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 don't think we need them at all. I believe everything should work even if both of these are undefined.
(They are only needed if you want to run other commands using netlify cli (that deploys code with pushing to git etc. Or download config files set up on the netlify UI etc.)
@@ -8,8 +8,6 @@ PH_DB_PORT=5432 | |||
|
|||
S3_VCF_FILE_URL="s3://phenopolis-vcf/August2019/merged2.vcf.gz" |
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.
We should replace this with a link to a local file
(or ideally an S3 link to a file on a local MinIO instance...)
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.
Let me merge this PR first and I will start working properly in #376.
Agree should point to local file as first time users are unlikely to have
AWS or may not be allowed to put data on AWS.
…On Fri, 29 Oct 2021, 07:51 Ismail Moghul, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In public.env
<#375 (comment)>
:
> @@ -8,8 +8,6 @@ PH_DB_PORT=5432
S3_VCF_FILE_URL="s3://phenopolis-vcf/August2019/merged2.vcf.gz"
We should replace this with a link to a local file
(or ideally an S3 link to a file on a local MinIO instance...)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#375 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5MN5FMSZ2WGMIK5JYU6RLUJJG7PANCNFSM5AM3HGNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Considering it to MERGE
I've completed the migration. AFAIK the only tables lingering from old schema (
public
) are actually actively used in the new schema:users
user_config
users_individuals
What should we do about them? Whatever decided, to be dealt in another PR.