-
Notifications
You must be signed in to change notification settings - Fork 8
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
Seeds Rework #174 #175
Seeds Rework #174 #175
Conversation
Codecov Report
@@ Coverage Diff @@
## main #175 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 24 +1
Lines 539 572 +33
=========================================
+ Hits 539 572 +33
Continue to review full report at Codecov.
|
@@ -1,9 +1,15 @@ | |||
export [email protected] | |||
export AUTH_API_KEY=2PzB7PPnpuLsbWmWtXpGyI+kfSQSQ1zUW2Atz/+8PdZuSEJzHgzGnJWV35nTKRwx/dwylauth.herokuapp.com | |||
export EMAIL_APP_URL=https://dwylmail.herokuapp.com | |||
export AUTH_URL=dwylauth.herokuapp.com |
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.
Had to add the AUTH_URL
environment variable to avoid the circular dependency on auth_plug
noted by @tadasajon in #170
@SimonLab please take a look at this PR and LMK what you think. Feedback very much welcome! |
# to setup the database: | ||
# | ||
# mix ecto.setup | ||
defmodule Auth.Seeds do |
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 bulk of this code moved to init.ex
above. ⬆️
def main do | ||
Logger.info("Initialising the Auth Database ...") | ||
# check required environment variables: | ||
Envar.is_set_all?(~w/ADMIN_EMAIL AUTH_URL ENCRYPTION_KEYS SECRET_KEY_BASE/) |
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 could create the list of environment variables as a module variable to make it easier to change later on if required. However I think it does the job at the moment
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.
@SimonLab yeah, that would be a great future enhancement. 👍
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.
Looks good to me, I've just added a couple of comments, let me know what you think @nelsonic but happy to merge otherwise
Co-authored-by: Simon <[email protected]>
@SimonLab thanks very much for reviewing. I've applied your suggested change to |
In order to run the seeds on Fly.io #172 / #173 we needed to move the bulk of the code to
/lib
that's what this PR does.I've tried to minimise the changes but had to update a few tests to keep it all 100%. See: #174