Dockerfile and docker-compose config updated due to package.json location changes #4882

benosteen · 2019-06-04T20:57:58Z

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Updated the Dockerfile and entrypoint.sh script to reference and use the new location for the .yarnrc and package.json files for yarn install and related calls. Also, updates the docker-compose.yml file to include the postgresql setup script required to set up the database template and extensions for Arches projects, as the new setup script no longer attempts to perform this super user action

Issues Solved

#4879

Checklist

Unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

…changes re #4879

mradamcox

I'm not able to test the docker side of this, but the sql init looks good to me, and is a good step toward further closing the dependency installation gap.

Also, eventually we'll be able to take the _20 off of the end of the postgis template db name (it's tied to the default arches settings.py file right now so I think that's a change for another time).

veuncent · 2019-06-04T21:10:30Z

Dockerfile

@@ -130,9 +130,12 @@ RUN . ../ENV/bin/activate \
    && pip install -e . --no-binary :all:

 # Install Yarn components
-COPY ./package.json ${ARCHES_ROOT}/package.json


This copy command was initially used to run yarn install before copying the complete code base into the container (COPY . ${ARCHES_ROOT}). The advantage of that is you don't have to run yarn install every time you make little changes to the code base (unless package.json changes).

There has been some changes to where and how this is done in master. #4878 which broke the automated setup in docker and the entrypoint. The line is not necessary, as package.json is now in the arches/install directory and copied over in a previous command, along with the yarnrc, etc all within the install dir again.

What I meant to say was that in the current order of commands it seems that every time a developer makes a change to any file in the ${ARCHES_ROOT} folder, the Docker build will have to run the yarn install (and also pip install) command. This should not be necessary of you run those before doing COPY . ${ARCHES_ROOT}. Those steps will then be cached and reused the next time you build the image.

(And for that to work you need to copy package.json separately from COPY . ${ARCHES_ROOT} ).

So add a copy of package.json, yarn.lock, and .yarnrc to the /arches/install dir and run yarn install before the COPY . ${ARCHES_ROOT} step?

I've just pushed the change to move the yarn install higher, to help it be cached as discussed.

veuncent · 2019-06-04T21:17:32Z

docker-compose.yml

@@ -68,6 +68,7 @@ services:
      volumes:
          - postgres-data:/var/lib/postgresql/data
          - postgres-log:/var/log/postgresql
+          - ./arches/install/init.sql:/docker-entrypoint-initdb.d/init.sql # to set up the DB template


Not sure if this is the right solution, but I may be missing something (it's late here). What if you have a database running in aws? Would you need to manually run this script on that db before you can let the Arches container run setup_db? Could this script be incorporated in setup_db instead?

This is letting the postgres container set up the template that Arches projects use to base themselves on. If the script was in setup_db, then the Arches container would have to have superuser postgresql credentials to be able to add the two extensions, which is something that should only happen once. For context, by IT policy, no app will ever get that sort of level of access to the provisioned DB. Having the required template be installed as part of deployment/provisioning, rather than when an app is being set up is not a big change, but is pretty crucial in a shared environment.

A fallback could be the entrypoint.sh looking for superuser creds in the environment variables, and running the sql script if they are present?

This addition to docker-compose is to allow for local deployment with pg in a 'nearby' container under the control of docker-compose.

Got it. Not forcing the use of superuser credentials seems a good move to me.

I'm still not entirely clear on how the deployment process will go on a non-local environment. As far as I can remember, currently when running Arches in a clean environment with an empty, newly provisioned Postgres db, all the user needs to do is run the Arches container and the db will be setup in a completely automated way. Would we lose this convenience with this new Postgis extensions script?

I do think your idea of looking for superuser credentials and running the script sounds good, so that at least we have a simpler (yet less secure) way to get people started. Could that be incorporated into this PR?

By the way, I do not see any directions regarding this provisioning script in the regular installation manual:
https://arches.readthedocs.io/en/stable/installation/
Should this be mentioned there?

Hi @veuncent , yes, it should definitely be mentioned there. I actually have a whole lot of install-related docs tickets grouped together in a project because I think a big overhaul of that process is warranted at this point. https://github.com/archesproject/arches-docs/projects/1

However, this change is not deployed yet in any published release, so if anything it should just be in the latest docs, not the master ones.

…p to init-unix.sql

mradamcox · 2019-06-04T21:35:20Z

arches/install/init.sql

@@ -0,0 +1,11 @@
+CREATE DATABASE template_postgis_20 WITH ENCODING 'UTF8' LC_COLLATE='en_US.utf8' LC_CTYPE='en_US.utf8';


When I tried this on windows, it failed saying that the locale name was invalid. Turns out Windows encoding is completely different English_United States.1252. So I recommend leaving this file as is, but renaming it something like init-unix.sql.

benosteen · 2019-07-01T21:28:34Z

@veuncent do you have any issue with this PR? It might be sensible to get the docker run and install steps working again as it is currently broken, and then addressing any further issue with a new issue/PR?

chiatt · 2019-07-09T18:13:32Z

I'm going to merge this PR and as @benosteen mentioned, we can write a new issue for any additional changes.

veuncent · 2019-07-10T14:51:46Z

Sorry, I was on holiday. I will look at the changes asap, @benosteen

…

On Tue, Jul 9, 2019 at 8:13 PM Cyrus Hiatt ***@***.***> wrote: Merged #4882 <#4882> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4882?email_source=notifications&email_token=AB4HCW3VV3CP5ZZUT7MRSS3P6TINJA5CNFSM4HTDBKD2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSM6PGGA#event-2470245144>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4HCW43ONOWHT4OUHVCTALP6TINJANCNFSM4HTDBKDQ> .

benosteen added 2 commits June 4, 2019 13:51

tweaked the Dockerfile and docker-compose config due to package.json …

afab7c3

…changes re #4879

Fixed typo in print statement

4971457

benosteen requested review from veuncent and mradamcox June 4, 2019 20:57

benosteen requested a review from a team as a code owner June 4, 2019 20:57

mradamcox approved these changes Jun 4, 2019

View reviewed changes

veuncent reviewed Jun 4, 2019

View reviewed changes

mradamcox self-requested a review June 4, 2019 21:28

Due to locale errors on Windows, renaming the default postgresql setu…

3bbdfb2

…p to init-unix.sql

mradamcox approved these changes Jun 4, 2019

View reviewed changes

veuncent mentioned this pull request Jun 6, 2019

python manage.py migrate requires postgres superuser account #4707

Closed

Shifted the yarn install layers to aid caching of container build #4879

9d38ea1

This was referenced Jul 3, 2019

docker-compose*.yml : bump versions of postgres and elasticsearch #4753

Closed

Dockerfile is broken in stable/4.4.2 release #4971

Open

chiatt merged commit 47bf804 into master Jul 9, 2019

chiatt deleted the 4879_fix_docker branch December 4, 2019 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dockerfile and docker-compose config updated due to package.json location changes #4882

Dockerfile and docker-compose config updated due to package.json location changes #4882

benosteen commented Jun 4, 2019

mradamcox left a comment

veuncent Jun 4, 2019

benosteen Jun 4, 2019

veuncent Jun 6, 2019

benosteen Jun 6, 2019

veuncent Jun 6, 2019

benosteen Jun 11, 2019

veuncent Jun 4, 2019

benosteen Jun 4, 2019

benosteen Jun 4, 2019

veuncent Jun 6, 2019

mradamcox Jun 6, 2019

mradamcox Jun 4, 2019

benosteen commented Jul 1, 2019

chiatt commented Jul 9, 2019

veuncent commented Jul 10, 2019 via email

		@@ -0,0 +1,11 @@
		CREATE DATABASE template_postgis_20 WITH ENCODING 'UTF8' LC_COLLATE='en_US.utf8' LC_CTYPE='en_US.utf8';

Dockerfile and docker-compose config updated due to package.json location changes #4882

Dockerfile and docker-compose config updated due to package.json location changes #4882

Conversation

benosteen commented Jun 4, 2019

Types of changes

Description of Change

Issues Solved

Checklist

mradamcox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benosteen commented Jul 1, 2019

chiatt commented Jul 9, 2019

veuncent commented Jul 10, 2019 via email