omgnetwork / ewallet

eWallet Backend for the OmiseGO SDKs.
https://omisego.network/
Apache License 2.0
324 stars 74 forks source link

mix seed shouldn't error if the BASE_URL wasn't set #98

Closed sirn closed 6 years ago

sirn commented 6 years ago

Subject of the issue

mix seed shouldn't error if the BASE_URL wasn't set.

Your environment

$ uname -a
Linux srv1 4.4.86+ #1 SMP Thu Dec 7 20:11:11 PST 2017 x86_64 GNU/Linux

Steps to reproduce

Leave out BASE_URL when running mix seed

Expected behavior

Seed works with URL fallback to example.com or something.

Actual behavior

Without BASE_URL:

$ env |grep ^BASE_URL
$ mix seed
** (ArgumentError) argument error
    apps/ewallet/priv/repo/report_minimum.exs:5: (file)
    (elixir) lib/code.ex:334: Code.load_file/2

With BASE_URL:

$ env BASE_URL=http://www.example.com/ mix seed
...snip...
Database seeding completed. Enjoy!
sirn commented 6 years ago

Also BASE_URL handling should properly handle URL with trailing slash and one without. Currently this is causing the URL to be generated as http://www.example.com//path (double /) in some place while running seeds.

unnawut commented 6 years ago

I'm going to rethink a bit about the whole developer experience setting up the eWallet. So I'll separate this into 2 parts:

  1. Fix the exception. Will do now.
  2. See if there's a better way to manage env vars / sanity check for missing env vars before allowing commands to run. This is for this week or next.
unnawut commented 6 years ago

@sirn Can you find out what Mix.env it's being run on? I'm curious because while :prod doesn't have a default :base_url value, :dev and :test environment do:

config :ewallet_db,
  base_url: System.get_env("BASE_URL") || "http://localhost:4000"

Also... do you think defaulting to https://localhost:4000 should this apply to :prod as well? In the current code, :prod environment is the one that does not have a default.

sirn commented 6 years ago
$ echo $MIX_ENV
prod

However, we should probably set this default only for seeds. Since seeds is a one-off script, and we should never assume it will have all environment variables set, especially if the environment variable was only for generating report output. In this case, example.com is a much better choice.

In other words, let's handle default here for seeds :-)

https://github.com/omisego/ewallet/blob/5f45118f4d282a4e7cdf668e7c242cf176b0b567/apps/ewallet/priv/repo/report_minimum.exs#L5