search

decrypto-org / blockchain-course

An interactive course on blockchain science and engineering
MIT License
14 stars 11 forks source link

Simplify and automate installation #19

Closed OrfeasLitos closed 5 years ago

OrfeasLitos commented 5 years ago

Fixes #17

OrfeasLitos commented 5 years ago

I'll add a note saying that any db works, but we provide instructions for postgres. In my opinion, it would be an omission not to include some example instructions.

cnasikas commented 5 years ago

I'll add a note saying that any db works, but we provide instructions for postgres. In my opinion, it would be an omission not to include some example instructions.

Instructions is ok! I just think that the script should be as agnostic as possible.

OrfeasLitos commented 5 years ago

There's a tradeoff between the script being agnostic and the user having to understand too much to install. (The 3rd option is to use much more complex things on our side, but that's something I won't be able to do too soon.)

OrfeasLitos commented 5 years ago

I also annotated the code example to use javascript highlighting.

cnasikas commented 5 years ago

The "Homepage URL" and "Authorisation callback URL" is environment depended. As Decrypto we have different configuration for development and production.

Develepment (blockchain-course-dev):

Homepage URL: http://localhost:3001/

Authorisation callback URL: http://localhost:3000/api/auth/github/callback

Production (blockchain-course):

Homepage URL: https://blockchain-course.org

Authorisation callback URL: https://blockchain-course.org/api/auth/github/callback

The "Homepage URL" in installation instructions of step 2 for development environment is wrong. It must be http://localhost:3001/ (the default)

gtklocker commented 5 years ago

General comment: you can use git subtrees instead of submodules to avoid having the extra step

OrfeasLitos commented 5 years ago

Fixed

cnasikas commented 5 years ago

Nice! I really believe that the db creation command should not be part of the installation script. None of the big frameworks such us Django, Laravel, Ruby on Rails, WordPress etc do that. They create databases, tables etc but the framework is agnostic to the underlying database. Nevertheless, its a very good PR so if you want you can merge as it is.

dionyziz commented 5 years ago

FWIW I agree with @cnasikas on the point of database creation. The current system is DBMS-agnostic. If we make it possible to work with a stand-alone database environment (SQLite), then it would make sense to generate the database in the script. For now it's best if this is a separate command ran by the user.

However, I do understand the merit of putting it in, in terms of ease-of-use. In that case, we should at least ask the user if they're using Postgres and if they want the database with the particular name to be created for them.

OrfeasLitos commented 5 years ago

Removed db creation from script, gave generic & specific instructions in README.md