Skip to content
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

If database.json includes {"ENV": "DATABASE_URL"} then migrations require the environment variable to contain a valid value #391

Closed
erezny opened this issue Jul 20, 2016 · 11 comments
Labels

Comments

@erezny
Copy link

erezny commented Jul 20, 2016

I defined a dev environment that uses sqlite3 and defined staging and prod environment which use heroku postgres. When I run a migration in the dev environment node-db-migrate complains about the environment variables in staging and prod. I do not want to include the flat values in my database.json because I do not want these secret values in my code base.

database.json:

{
  "defaultEnv": "dev",
  "dev": {
    "driver": "sqlite3",
    "filename": "dev.db"
  },

  "test": {
    "driver": "sqlite3",
    "filename": ":memory:"
  },

  "staging": { "ENV": "DATABASE_URL" },
  "prod": { "ENV": "DATABASE_URL" }
}
$ node node_modules/db-migrate/bin/db-migrate up -e dev --config "./database.json" -v
[INFO] Detected and using the projects local version of db-migrate. '/.../node_modules/db-migrate/index.js'
[ERROR] TypeError: Parameter "url" must be a string, not undefined
    at Url.parse (url.js:87:11)
    at Object.urlParse [as parse] (url.js:81:5)
    at module.exports (/.../node_modules/parse-database-url/lib/parse-database-url.js:16:23)
    at Object.exports.loadObject (/.../node_modules/db-migrate/lib/config.js:67:18)
    at Object.exports.loadFile (/.../node_modules/db-migrate/lib/config.js:59:18)
    at loadConfig (/.../node_modules/db-migrate/api.js:555:18)
    at Object.dbmigrate (/.../node_modules/db-migrate/api.js:67:17)
    at Object.module.exports.getInstance (/.../node_modules/db-migrate/index.js:56:10)
    at /.../node_modules/db-migrate/bin/db-migrate:34:23
    at /.../node_modules/resolve/lib/async.js:44:21
    at ondir (/.../node_modules/resolve/lib/async.js:187:31)
    at onex (/.../node_modules/resolve/lib/async.js:93:22)
    at /.../node_modules/resolve/lib/async.js:24:18
    at FSReqWrap.oncomplete (fs.js:123:15)
@wzrdtales
Copy link
Member

Hey @erezny,

essentially, your first problem is that you seem to have the database.json commited in your repository. This is something you should never want to do for any config files.

In fact, db-migrate allows configuring it via the DATABASE_URL environment variable. There is no need to define a configuration file when DATABASE_URL is defined.

So what you did defined here, does actually have no effect at all:

  "prod": { "ENV": "DATABASE_URL" }

Take a look here:

https://github.com/db-migrate/node-db-migrate/blob/master/api.js#L550-L551

You can see that DATABASE_URL if defined will override everything else. This is the expected behavior and also mentioned in the documentation:

Alternatively, you can specify a DATABASE_URL environment variable that will be used in place of the configuration file settings. This is helpful for use with Heroku.

So you see, that the heroku use case is also already built in and all you need is a valid database uri. Also read https://devcenter.heroku.com/articles/heroku-postgresql#create-a-new-database for reference.

@wzrdtales
Copy link
Member

If you have any other questions, please feel free to reach out.

@wzrdtales
Copy link
Member

Btw. another addition, just from years of experience, you should work in development on the same environment like in production if you don't want any bad surprises.

@erezny
Copy link
Author

erezny commented Jul 21, 2016

Right, OK.

@erezny erezny closed this as completed Jul 21, 2016
@fklingler
Copy link

I think this should still be fixed. Other env variables names can be used.
I have the exact same error with this config file:

{
  "defaultEnv": "development",

  "development": "mongodb://localhost/myproject-dev",
  "production": {"ENV": "MONGO_URL"}
}

The only thing that causes this error is that my MONGO_URL env variable is not set in development, and I don't see any reason why that shouldn't be possible.

@wzrdtales
Copy link
Member

@fklingler Ok, but you talk about a completely different thing now.
DATABASE_URL is something completely different from {"ENV": "MONGO_URL"}.

Maybe you could open a new issue on this and describe more your problem and give a way to reproduce your problem? Currently I don't really know where your problem is though, I would need a bit more details though.

@fklingler
Copy link

I still think this is the same exact problem.

On my production environment, I set the database URL using an environment variable, in this case, MONGO_URL.
On my development environment, I set the database URL from config files (a node module in my express app, or here in the json file).

On this development environment, I don't have the MONGO_URL env variable set. This is what causes the problem.
Apparently, db-migrate iterates over everything (even the non-current environment) in the database.json file, and if there is something in the form of {"ENV": "XXXX"}, it will try to parse process.env.XXXX using parse-database-url.
Since process.env.XXXX is undefined in some cases (potentially all the other environments that are not using {"ENV": "XXXX"} in the config) and parse-database-url doesn't accept undefined, it throws this error.

Is this more clear?
If you still think this is another problem, I will gladly open another issue!

@wzrdtales
Copy link
Member

wzrdtales commented Sep 9, 2016

@fklingler It is, as said, those are two complete different things. They're actually handled different.

DATABASE_URL is a standardized database_url, used by many cloud providers. But MONGO_URL is just an environment variable, not more. I'll come back to you in about 20 minutes, I'm on the go currently.

@fklingler
Copy link

This is exactly the same problem, but the solution is just simpler if the environment variable in question is DATABASE_URL since we can remove its reference in the database.json config file.

I will also go now, I'll be back in a few hours or tomorrow. Thanks for your quick answers!

@wzrdtales
Copy link
Member

@fklingler Ok, back a bit earlier than thought.

So yes and no. While you're right the problem is kind of the same and that the behavior you describe is not wanted, those still are two different problem. The thing is that you don't need and should not define the DATABASE_URL anywhere in your config. This was the issue of this issue actually. So I just would like to keep those two things as separate issues. I have reproduced your behavior now already though and put it on my list. I will open a separate issue for this later, if I did not did this when you come back, feel free to open it instead of me :)

@sanjaypandey786
Copy link

DATABASES = {
'default': ENV_JSON['default'],
'data': ENV_JSON['data'],
jsonerror
jsonerror

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants