Skip to content

Dockerize#6

Merged
dorukozturk merged 5 commits intomasterfrom
dockerize
Oct 9, 2019
Merged

Dockerize#6
dorukozturk merged 5 commits intomasterfrom
dockerize

Conversation

@dorukozturk
Copy link
Copy Markdown
Contributor

No description provided.

@dorukozturk dorukozturk requested a review from matthewma7 October 8, 2019 00:20
Copy link
Copy Markdown

@matthewma7 matthewma7 left a comment

Choose a reason for hiding this comment

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

This worked for me. 😀 Have left two questions.

Comment thread docker/nginx.conf
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-NginX-Proxy true;
proxy_pass http://girder/api/;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is nginx also proxying girder? so I am wondering if we need to set the cors stuff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just took the config that Brandon did. I think we still need cors stuff.

sleep 10
python3 /home/provision/girder.py
cd /home/data && python3 ingest.py ./data girder 8080 admin letmein
cd /home/data && python3 ingest_1000.py ./data girder 8080 admin letmein
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We probably don't need to provision each time we start the container, right? Maybe just leave them out and run them manually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The scripts are idempotent so running it multiple times is quick and yields the same result. I wanted to do everything as part of docker deployment so that we do not have to run manual steps. I don't have a preference this just seemed easier for deployment :).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The data ingestion is kind of two heavy as part of the startup. I am thinking that data is not part of application. I think we will have more data, we don't want to always copy them to the docker image. The beauty of girder client-based data ingestion is that we can run it from anywhere and the original data don't have to be exists on the server.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can make that change so that ingest scripts are not part of provisioning but a post provisioning step that we do manually. I will make that change soon.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cool! and all the parameterize work is useful either way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@matthewma7 I addressed your comment. PTAL

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cool. Thanks!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We could merge this one now.

@dorukozturk dorukozturk merged commit fa84735 into master Oct 9, 2019
@dorukozturk dorukozturk deleted the dockerize branch October 9, 2019 13:22
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.

2 participants