chore: run container script run through CLI args #6690

codedsun · 2019-12-22T20:02:18Z

Fixes #6598

Short description of what this resolves:

Container script now can be run by CLI args instead of envt variables

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
The 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)

All the functions created/modified in this PR contain relevant docstrings.

codecov · 2019-12-22T20:06:25Z

Codecov Report

Merging #6690 into development will decrease coverage by <.01%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #6690      +/-   ##
===============================================
- Coverage        65.44%   65.43%   -0.01%     
===============================================
  Files              300      300              
  Lines            15299    15299              
===============================================
- Hits             10012    10011       -1     
- Misses            5287     5288       +1

Impacted Files	Coverage Δ
app/models/event.py	`78.57% <0%> (-0.36%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a71e47...6c3e7a3. Read the comment docs.

codedsun · 2019-12-22T20:07:07Z

@iamareebjamal Please review.

iamareebjamal · 2019-12-22T20:42:43Z

docker-compose.yml

      C_FORCE_ROOT: "true"
+    command: run celery


Let's just use celery, instead of run celery

And api should start when no argument is passed and also when api argument is passed

Ok doing the change.

codedsun · 2019-12-23T12:11:03Z

@iamareebjamal Check now, done the changes! Also we have to pass this command in DockerFile while starting script, correct?

codedsun · 2019-12-23T12:27:21Z

Dockerfile

@@ -26,4 +26,4 @@ WORKDIR /data/app
 ADD . .

 EXPOSE 8080
-CMD ["sh", "scripts/container_start.sh"]
+CMD ["sh", "scripts/container_start.sh run "]


@iamareebjamal How do we pass the command from celery container here?

Change this to ENTRYPOINT

iamareebjamal · 2019-12-23T13:51:05Z

scripts/container_start.sh

+  echo "[LOG] Using database: ${DATABASE_URL}"
+  echo "[LOG] Using redis: ${REDIS_URL}"
+
+  if [ "$2" = "" ] || [ "$2" = "api" ]; then


See how default parameters are handled above

iamareebjamal · 2019-12-23T13:51:27Z

scripts/container_start.sh

-
-if [ "$DEPLOYMENT" == "api" ]
+if
+  [ "$1" = "run" ]


Remove as told. No need for run

iamareebjamal · 2019-12-23T15:24:31Z

scripts/container_start.sh

@@ -1,12 +1,11 @@
 #!/bin/sh
-export DEPLOYMENT=${DEPLOYMENT:-api}
+DEPLOYMENT=${1:-api}


Why removed export?

why we want it to be global?

So that we can know inside the container which one it is

iamareebjamal · 2019-12-23T15:25:21Z

scripts/container_start.sh


 echo "[LOG] Deploying ${DEPLOYMENT}"
 echo "[LOG] Using database: ${DATABASE_URL}"
 echo "[LOG] Using redis: ${REDIS_URL}"

-if [ "$DEPLOYMENT" == "api" ]
-then
+if [ "$DEPLOYMENT" = "api" ]; then


iamareebjamal · 2019-12-23T15:25:28Z

scripts/container_start.sh

-if [ "$DEPLOYMENT" == "celery" ]
-then
+
+if [ "$DEPLOYMENT" = "celery" ]; then


iamareebjamal · 2019-12-23T15:25:56Z

Tested?

codedsun · 2019-12-23T15:53:50Z

Not working for celery while running docker-compose up

codedsun · 2019-12-23T16:54:35Z

@iamareebjamal the docker-compose is directly running celery instead of using it in entrypoint

Log

opev-web    | [LOG] Using redis: redis://redis:6379/0
opev-web    | [LOG] Waiting for Database
opev-web    | [LOG] Database Up
opev-web    | [LOG] Preparing database
opev-celery | usage: celery <command> [options] 
opev-celery | 
opev-celery | Show help screen and exit.
opev-celery | 
opev-celery | positional arguments:
opev-celery |   args
opev-celery | 
opev-celery | optional arguments:
opev-celery |   -h, --help

codedsun · 2019-12-23T17:39:46Z

@iamareebjamal Ref: https://stackoverflow.com/a/40843513/6026355

it says so, where I am wrong

iamareebjamal · 2019-12-23T17:47:52Z

Are you building the image or just using the eventyay image

codedsun · 2019-12-23T17:48:39Z

Are you building the image or just using the eventyay image

I am not sure about this, how to know?. I only ran docker-compose up

codedsun · 2019-12-23T17:51:02Z

shit I was using the eventyay image

iamareebjamal · 2019-12-23T17:51:21Z

Then you're not building the image, and obviously it won't work. Remove image: tag in docker compose file in default section and replace it with build: .

Locally, don't commit and push

codedsun · 2019-12-23T18:02:41Z

@iamareebjamal still getting errors

Successfully tagged open-event-server_celery:latest
WARNING: Image for service celery was built because it did not already exist. To rebuild this image you must use `docker-compose build` or `docker-compose up --build`.
Creating opev-postgres ... done
Creating opev-redis    ... done
Creating opev-celery   ... error
Creating opev-web      ... 

Creating opev-web      ... error

ERROR: for opev-web  Cannot start service web: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"scripts/container_start.sh\": permission denied": unknown

ERROR: for celery  Cannot start service celery: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"scripts/container_start.sh\": permission denied": unknown

ERROR: for web  Cannot start service web: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: \"scripts/container_start.sh\": permission denied": unknown
ERROR: Encountered errors while bringing up the project.

codedsun · 2019-12-23T18:07:26Z

solution says to change the permission of bash file
ref: https://stackoverflow.com/a/46353378/6026355

iamareebjamal · 2019-12-23T18:14:58Z

That's because you needlessly changed ['sh', 'entrypoint'] to ['entrypoint']

As I have said countless times now, copy it as is until you understand it. DO NOT change something if you are not 100% sure why you're doing it. No idea why you make such changes

codedsun · 2019-12-23T18:21:40Z

@iamareebjamal I am sorry but making mistakes are part of learning, as you said to change the
CMD ["sh", "scripts/container_start.sh"] to
ENTRYPOINT here #6690 (comment)

So what should be ENTRYPOINT ["command"] ?
I read this format of the ENTRYPOINT.

ENTRYPOINT ["sh", "scripts/container_start.sh"]

I am again sorry, but without the knowledge of docker. I would not have taken this task might be.

iamareebjamal · 2019-12-23T18:23:05Z

Then it should have been just

-CMD
+ENTRYPOINT

codedsun · 2019-12-23T18:24:04Z

do i need to remove the images or running docker-compose up --force-recreate will work for checking?

codedsun · 2019-12-23T18:28:19Z

I am using docker for the first time, please ignore my mistakes as a learner. I had a hard time to understood this but I will learn it by working more on it. @iamareebjamal

Also, I am very excited by how the image process worked, it was amazing.

Thanks a lot! Everything working fine. Tested

codedsun · 2019-12-23T18:31:07Z

tested ss:

codedsun · 2019-12-23T18:34:17Z

@iamareebjamal Review please.

auto-label bot added the chore label Dec 22, 2019

codedsun mentioned this pull request Dec 22, 2019

chore: container script run through CLI args #6599

Closed

6 tasks

iamareebjamal reviewed Dec 22, 2019

View reviewed changes

codedsun force-pushed the container-2 branch from fa8abd4 to 5a0e2fa Compare December 23, 2019 12:07

codedsun force-pushed the container-2 branch from 5a0e2fa to d095df7 Compare December 23, 2019 12:25

codedsun commented Dec 23, 2019

View reviewed changes

iamareebjamal reviewed Dec 23, 2019

View reviewed changes

codedsun force-pushed the container-2 branch from d095df7 to b3e774a Compare December 23, 2019 14:13

codedsun requested a review from iamareebjamal December 23, 2019 14:26

iamareebjamal reviewed Dec 23, 2019

View reviewed changes

scripts/container_start.sh Outdated

if [ "$DEPLOYMENT" == "celery" ]

then

if [ "$DEPLOYMENT" = "celery" ]; then

Copy link

Member

iamareebjamal Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

codedsun force-pushed the container-2 branch 2 times, most recently from ae17595 to bbf3e93 Compare December 23, 2019 15:32

chore:run container script run through CLI args

6c3e7a3

codedsun force-pushed the container-2 branch from bbf3e93 to 6c3e7a3 Compare December 23, 2019 18:29

iamareebjamal changed the title ~~chore:run container script run through CLI args~~ chore: run container script run through CLI args Dec 23, 2019

iamareebjamal approved these changes Dec 23, 2019

View reviewed changes

iamareebjamal merged commit 03fa0ea into fossasia:development Dec 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: run container script run through CLI args #6690

chore: run container script run through CLI args #6690

codedsun commented Dec 22, 2019

codecov bot commented Dec 22, 2019 •

edited

Loading

codedsun commented Dec 22, 2019

iamareebjamal Dec 22, 2019

iamareebjamal Dec 22, 2019

codedsun Dec 23, 2019

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun Dec 23, 2019

iamareebjamal Dec 23, 2019

iamareebjamal Dec 23, 2019

iamareebjamal Dec 23, 2019

codedsun Dec 23, 2019

iamareebjamal Dec 23, 2019

codedsun Dec 23, 2019

iamareebjamal Dec 23, 2019

iamareebjamal Dec 23, 2019

iamareebjamal Dec 23, 2019

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

chore: run container script run through CLI args #6690

chore: run container script run through CLI args #6690

Conversation

codedsun commented Dec 22, 2019

Short description of what this resolves:

Checklist

codecov bot commented Dec 22, 2019 • edited Loading

Codecov Report

codedsun commented Dec 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun commented Dec 23, 2019 • edited Loading

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

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019 • edited Loading

codedsun commented Dec 23, 2019 • edited Loading

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019 • edited Loading

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

iamareebjamal commented Dec 23, 2019 • edited Loading

codedsun commented Dec 23, 2019 • edited Loading

iamareebjamal commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

codedsun commented Dec 23, 2019

codecov bot commented Dec 22, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

iamareebjamal commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading