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

#1340: added docker support for spinning up the dev environment #1364

Conversation

athersharif
Copy link
Collaborator

@athersharif athersharif commented Dec 15, 2018

fixes #1340.
fixes #483

this PR adds docker support for spinning up the dev environment. few notes:

  • this setup, essentially, only needs one single command to install all the dependencies and the libraries, and another to start the web server.
  • the dump is no longer needed for setting up the project. schema is checked in with this pr that will allow the barebone structure for the db necessary for the webpage to load. importing the dump would still be needed for dev needs, and a script (supplied in this pr) will make it easy to do so.
  • this pr also fixes an issue with the javax.media jai-core library (solution suggested by @aileenzeng here: Can't build website service aileenzeng/sidewalk-docker#5).
  • instructions are updated in readme.

@misaugstad
Copy link
Member

@athersharif I think instead of modifying all the evolutions files to add IF EXISTS, you should use the schema of the databases before the evolutions were applied. Then you should probably include a few pieces of data in addition to the schema, namely what was in the role table and label_type table.

I've only just started testing, but I figured I'd bring this up sooner rather than later :)

@athersharif
Copy link
Collaborator Author

@misaugstad the reason why I had to add if exists was because the schema does get imported before the evolutions are applied. so, when the evolutions start, we start seeing a bunch of "relation already exists" errors. the schema also comes with the needed data as you mentioned :) - https://github.com/ProjectSidewalk/SidewalkWebpage/pull/1364/files#diff-f66ab5a130a7ebca7bd1821de363bb02R549, https://github.com/ProjectSidewalk/SidewalkWebpage/pull/1364/files#diff-f66ab5a130a7ebca7bd1821de363bb02R1089, and https://github.com/ProjectSidewalk/SidewalkWebpage/pull/1364/files#diff-f66ab5a130a7ebca7bd1821de363bb02R1534.

@misaugstad
Copy link
Member

@athersharif

the reason why I had to add if exists was because the schema does get imported before the evolutions are applied. so, when the evolutions start, we start seeing a bunch of "relation already exists" errors

Yep, that's why I suggest creating a schema from a version of the database where the evolutions haven't been applied to it. The alternative would be to use the schema you have, but include the entries in the play_evolutions in the same way you included the label_type and role entries (this will make it so that the evolutions are not applied). I think it is cleaner to use a schema where the evolutions have not been applied yet, if possible.

@athersharif
Copy link
Collaborator Author

@misaugstad ah, right. totally get what you're saying. how do you feel about the schema handling everything besides the data, and the evolutions handling the data part? because tbh, adding inserts into the schema kinda makes it not the schema anymore 🤣

there's also the issue of adding the fields in the evolutions but they get added as part of the schema anyway (and maybe, by definition of a schema, should). I hacked around it by removing those fields in the evolutions (don't really like what I did there) before the add statements are executed.

proposition to move all the creates in schema and all the alters and inserts into the evolutions?

@misaugstad
Copy link
Member

there's also the issue of adding the fields in the evolutions but they get added as part of the schema anyway (and maybe, by definition of a schema, should). I hacked around it by removing those fields in the evolutions (don't really like what I did there) before the add statements are executed.

This should also be solved by creating the schema from a database before the evolutions were applied (the schema would not include those columns).

proposition to move all the creates in schema and all the alters and inserts into the evolutions?

I don't think that we should be modifying the evolutions files. The purpose of them is to make changes to a live production database, and they are not meant to ever be modified. Like if we modified these evolutions files and pushed to a live database, it would apply the downs for all the evolutions (which would delete a bunch of tables that had data in them) then would reapply the modified evolutions.

I am not 100% rigid on leaving the evolution files alone, but I think we should do as much as we can to avoid touching them. Even if it involves bending the definition of a "schema" 😉

@athersharif
Copy link
Collaborator Author

hmm, that clarifies things a bit. in that case, yeah we should probably generate the schema-only dump from the prod database, and let evolutions do the rest. we'd just need to move the inserts that i mentioned above into the evolutions, and we should be good to go on that front. (basically, just re-iterating your suggestion).

thoughts on the other bits?

@misaugstad
Copy link
Member

we'd just need to move the inserts that i mentioned above into the evolutions

I still think we should keep the inserts in the "schema" (maybe we just shouldn't call it a "schema" 😉 )

thoughts on the other bits?

What are the other bits? 😅

@misaugstad
Copy link
Member

@athersharif from the console output it looks like you are using activator run instead of sbt run if that is the case, can you make sure to switch it to sbt run?

@athersharif
Copy link
Collaborator Author

@misaugstad yup, got it. by other bits, i meant were you able to run the server, etc. etc. don't wanna push new code when you're in the process of reviewing it. 🤣

yup, using activator run because that's how it's set up currently: https://github.com/ProjectSidewalk/SidewalkWebpage/blob/develop/package.json#L18. i can switch it to sbt run if you like but prob should do this in a quick follow up pr, imo (explaining whats the difference, why one over the other, and when one over the other).

@misaugstad
Copy link
Member

yup, got it. by other bits, i meant were you able to run the server, etc. etc.

Oh I see! Yes I just now finished the first compilation, and it runs! With the change you made plus using sudo.

yup, using activator run because that's how it's set up currently:

Oh interesting, when are those commands run right now? I didn't realize those were ever being run in the background.

The reason I say to use sbt over activator is that activator was EOL'd in 2017 😅 and sbt is the direct replacement.

@athersharif
Copy link
Collaborator Author

cool, i'll make the changes to the schema and the evolutions as per your suggestions, and add the sudo-for-linux instruction in the readme.

haha, wow just saw here. i'll switch to sbt and clean up the requirements for activator (will spin up the containers even faster).

@misaugstad
Copy link
Member

Amazing, thank you! Now let's say I exit after running npm start. And maybe I make some changes to the code. What should I run now? Do I run make dev and npm start or just npm start?

@athersharif
Copy link
Collaborator Author

my understanding of the codebase so far is this: if you make changes to the assets (js, css, etc.), grunt is already running and will pick that up. any changes to the core engine would need a rebuild - Ctrl+D to stop the server and rerun npm start should suffice but i'm sure we can optimize this. probably should add this in the readme too.

@misaugstad
Copy link
Member

any changes to the core engine would need a rebuild - Ctrl+D to stop the server and rerun npm start should suffice but i'm sure we can optimize this

Yep this is one of the main things we will be doing while doing dev. So this part needs to be tested and added to README. I didn't follow that exact process, but I am rerunning npm start after making a trivial change to the back-end, and now I think all the libraries are being re-downloaded, which definitely is not what we want 😁 But could you test by using the exact process you gave and make sure we don't re-download everything?

@athersharif
Copy link
Collaborator Author

yikes, yeah definitely don't want that. you got it, will push new commits shortly.

@jonfroehlich
Copy link
Member

jonfroehlich commented Dec 19, 2018 via email

@misaugstad
Copy link
Member

Still testing, but some stuff I've noticed:

  1. Your instructions for loading a database dump say to name the dump dump.sql but your script requires it to be named just dump.
  2. So there are problems (to be expected) with loading the schema. I think that we could make the "schema" work by creating the schema from an up-to-date version of the codebase, and then along with the label_type, mission_type, region_type, role, survey_category_option, survey_option, survey_question, and tag tables, you also include the inserts for the play_evolutions table. Then you won't need the inserts for the street_edge table I'd think. And with the play_evolutions table entries included, the application won't try to run all the evolutions. Sorry for running you all over the place for this 😅

@athersharif
Copy link
Collaborator Author

@misaugstad good catch on dump.sql!

a bit confused here. if i understand you correctly, is this what you're looking for:

  • generate the schema from the dump (which we are)
  • if the evolutions try to create the same table that the schema has already created, remove these queries from the evolutions (my initial commit was to use if exists but removing them altogether is probably much cleaner). remove them for downs too?
  • move the inserts from the evolutions for these tables into the schema file

did i miss anything above?

@misaugstad
Copy link
Member

Yes to first point, no to 2nd and 3rd point.

  1. Generate the schema from the dump
  2. Add inserts for the following tables into the schema: label_type, mission_type, region_type, role, survey_category_option, survey_option, survey_question, tag, and play_evolutions
  3. Leave the evolutions files untouched.

If you include the entries in the play_evolutions table in the schema, then the evolutions files will never be run, and can thus be left untouched by your PR.

@athersharif
Copy link
Collaborator Author

wouldn't that be an issue for all the renames in the evolutions though?

@athersharif
Copy link
Collaborator Author

additionally, here are some findings:

  • there doesn't seem to be an evolution for region_type.
  • there doesn't seem to a tag table in the schema.
  • its not entirely clear what entries play_evolutions accepts. for example, how is the hash generated? what are the values for state? etc.

now a dumb question - if the goal here is to avoid running the evolutions completely (as you said they'll never be run, with this change), why do they exist?

@misaugstad
Copy link
Member

wouldn't that be an issue for all the renames in the evolutions though?

How so if the evolutions are never run?

there doesn't seem to be an evolution for region_type

Including region_type doesn't have anything to do with the evolutions. It is just that the region_type is something that is static and the same for every database (same as label_type).

there doesn't seem to a tag table in the schema.

Okay what you want to do is run the develop branch with whatever database you want to create the schema from. This will cause the recently added evolutions to run, which will add that table. Then you can create the schema.

its not entirely clear what entries play_evolutions accepts. for example, how is the hash generated? what are the values for state? etc.

I'm not sure why that matters. You just need to make inserts for the records that were already in the database you create the schema from.

if the goal here is to avoid running the evolutions completely (as you said they'll never be run, with this change), why do they exist?

They are a bit of a legacy thing. They were added whenever we wanted to make changes to the code that involved changing the database structure. The idea is that the evolutions were already run, and you only run an evolution when you add a new evolution file.

@athersharif
Copy link
Collaborator Author

Okay what you want to do is run the develop branch with whatever database you want to create the schema from. This will cause the recently added evolutions to run, which will add that table. Then you can create the schema.

that's basically the answer to all my questions :). pushed the new commits to address the comments.

@misaugstad
Copy link
Member

Started fresh and on the npm start it hung here for a long time:

npm-start-issue

It did eventually continue, but it hung there for like 10 minutes I think. And idk if it ended up continuing because I refreshed the page or something.

That's as far as I was able to get tonight (haven't been able to test restoring a database dump, etc). Will get to that tomorrow.

@misaugstad
Copy link
Member

@athersharif there still seem to be some issues with the schema.sql.

Okay what you want to do is run the develop branch with whatever database you want to create the schema from. This will cause the recently added evolutions to run, which will add that table. Then you can create the schema.

It seems like you didn't do this exactly, because I am at least finding a bunch of tables in the schema that shouldn't be there. For example, the accessibility_feature and mission_user tables should have been dropped in different evolutions, but they are included in your schema.

@athersharif
Copy link
Collaborator Author

@misaugstad you're right. so bizarre that those stayed even though i ran the evolutions and then exported the schema. fixed them in 72aff2c.

@misaugstad
Copy link
Member

The schema looks good now! My last concern is the potential conflict in the build.sbt between what @athersharif 's and @aileenzeng 's docker-related PRs might cause. Could both of you maybe chat offline (possibly during the hackathon on Friday) about why you made the changes you made to the build.sbt, and hopefully decide what that file should ultimately look like?

@athersharif athersharif force-pushed the 1340-dev-env-added-docker-support-for-spinning-up-the-dev-environment branch from 7385526 to c34abb2 Compare January 29, 2019 08:02
@athersharif
Copy link
Collaborator Author

@misaugstad you're right. for some reasons, the schema was reflecting the changes from master. updated that in the last commit. (also, rebased on the develop branch so this pr is up to date with the develop branch).

@aileenzeng pushed the changes as we discussed today.

@misaugstad
Copy link
Member

@athersharif just tested the setup and everything seemed to work great! Changes to the back-end or front-end were auto-updated. Stopping npm start and then running it again was fast. And the database schema looks good (I used it to create our Newberg database!).

@aileenzeng could you test it one more time to make sure all those things you mentioned above have been fixed for you?

@olgarithm
Copy link
Collaborator

I've been doing my intro task using the Docker set-up, and Mikey asked me to comment that keeping Docker running on my computer drains my battery really quickly (to the point where I'm losing like 10% in 2 minutes).

@misaugstad
Copy link
Member

Just to finish with some relevant things @olgatron9000 told me: this incentivizes her to stop Docker, and restart it when she is working again. Starting it up then takes 5-10 minutes that first time.

@athersharif
Copy link
Collaborator Author

@olgatron9000 have you tried docker pause instead of docker stop? stopping the container sends a kill signal to the processes running on the container and restarting it would start the process again. pause on the other hand suspends them.

@aileenzeng
Copy link
Member

aileenzeng commented Feb 14, 2019

It works for me! I think the only comment that I had is that it took me a while to load the webpage after modifying a .js file (grunt took ~45 sec. to run, and it also took over 1 min. to load the webpage after grunt finished).

@jonfroehlich
Copy link
Member

jonfroehlich commented Feb 14, 2019 via email

@aileenzeng
Copy link
Member

Generally it takes less than 15 seconds (grunt + reloading the webpage)

@jonfroehlich
Copy link
Member

jonfroehlich commented Feb 14, 2019 via email

@misaugstad
Copy link
Member

@jonfroehlich how do you feel about merging the PR now and creating an issue for improving the update time for the dev environment?

@jonfroehlich
Copy link
Member

jonfroehlich commented Feb 15, 2019 via email

@athersharif
Copy link
Collaborator Author

sorry, i am just getting caught up on this. had family visiting from back home.

pretty positive this is a docker for mac issue. unfortunately, docker for mac is always behind the regular docker stuff and has some additional complications with respect to volumes and file systems that causes it to be slow and use a higher cpu consumption than normal. fortunately, there are workarounds for that. i'll push a new pr for the new issue created to address that.

@athersharif athersharif deleted the 1340-dev-env-added-docker-support-for-spinning-up-the-dev-environment branch February 15, 2019 22:29
@misaugstad misaugstad mentioned this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants