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

Add 'docker-compose' file #1163

Merged
merged 25 commits into from
Apr 23, 2024
Merged

Add 'docker-compose' file #1163

merged 25 commits into from
Apr 23, 2024

Conversation

toddgardner
Copy link
Contributor

This allows a run of a local development platform with 'docker-compose up' that runs both the database and server. The only tricky implementation is relying on the health checks as mysql takes a bit of time to startup.

Copy link
Contributor

@chaoticbear chaoticbear left a comment

Choose a reason for hiding this comment

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

No blockers from me if this actually is working. I just made a few notes on how my docker-compose setups are usually set up. Also we should probably add some documentation to the README

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@chaoticbear chaoticbear left a comment

Choose a reason for hiding this comment

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

A couple more quick suggestions. Also I tried to spin it up and couldn't log in. I'm not sure why. I can't remember if there's a command you need to run to seed the users from conf/directory.yaml or if it's supposed to just work.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@toddgardner
Copy link
Contributor Author

A couple more quick suggestions. Also I tried to spin it up and couldn't log in. I'm not sure why. I can't remember if there's a command you need to run to seed the users from conf/directory.yaml or if it's supposed to just work.

Sorry mentioned in my slack, I was thinking of seeding a database but I think I was just overcomplicating it; the current pushed version just uses the DMS file backend,and should work from the instructions in the repo:

    cp conf/directory-sample.yaml conf/directory.yaml

    docker-compose up

I've confirmed at least locally I can log in as Parenthetical.

@chaoticbear
Copy link
Contributor

A couple more quick suggestions. Also I tried to spin it up and couldn't log in. I'm not sure why. I can't remember if there's a command you need to run to seed the users from conf/directory.yaml or if it's supposed to just work.

Sorry mentioned in my slack, I was thinking of seeding a database but I think I was just overcomplicating it; the current pushed version just uses the DMS file backend,and should work from the instructions in the repo:

    cp conf/directory-sample.yaml conf/directory.yaml

    docker-compose up

I've confirmed at least locally I can log in as Parenthetical.

@toddgardner don't you also need to copy the conf/imsd.conf and adjust it for the docker DB settings?

@toddgardner
Copy link
Contributor Author

A couple more quick suggestions. Also I tried to spin it up and couldn't log in. I'm not sure why. I can't remember if there's a command you need to run to seed the users from conf/directory.yaml or if it's supposed to just work.

Sorry mentioned in my slack, I was thinking of seeding a database but I think I was just overcomplicating it; the current pushed version just uses the DMS file backend,and should work from the instructions in the repo:

    cp conf/directory-sample.yaml conf/directory.yaml

    docker-compose up

I've confirmed at least locally I can log in as Parenthetical.

@toddgardner don't you also need to copy the conf/imsd.conf and adjust it for the docker DB settings?

In the previous version when the above comment was written it was just relying on ENV vars in the docker-compose. In the one I just pushed I overwrote CMD in the docker-compose so I can pass the config file (and moved all settings into the imsd.conf that could be moved there, which is everything except IMS_DIRECTORY)

@chaoticbear
Copy link
Contributor

chaoticbear commented Apr 6, 2024

A couple more quick suggestions. Also I tried to spin it up and couldn't log in. I'm not sure why. I can't remember if there's a command you need to run to seed the users from conf/directory.yaml or if it's supposed to just work.

Sorry mentioned in my slack, I was thinking of seeding a database but I think I was just overcomplicating it; the current pushed version just uses the DMS file backend,and should work from the instructions in the repo:

    cp conf/directory-sample.yaml conf/directory.yaml

    docker-compose up

I've confirmed at least locally I can log in as Parenthetical.

@toddgardner don't you also need to copy the conf/imsd.conf and adjust it for the docker DB settings?

In the previous version when the above comment was written it was just relying on ENV vars in the docker-compose. In the one I just pushed I overwrote CMD in the docker-compose so I can pass the config file (and moved all settings into the imsd.conf that could be moved there, which is everything except IMS_DIRECTORY)

@toddgardner I didn't realize we could get away with not using the imsd.conf and just use env vars. For development at least I think that's better because then we can put all of our options in one .env file.

I re-based my PR on yours, take a look and if you don't hate it, go ahead and merge it into yours https://github.com/toddgardner/ranger-ims-server/pull/1

I added overrides for all of the env vars so that when we're tweaking things we can just adjust our local .env and not end up committing changes to the docker-compose. I also added networking which corresponds to the PR I have started on the web client burningmantech/ranger-ims-web#1038

Copy link
Contributor

@chaoticbear chaoticbear left a comment

Choose a reason for hiding this comment

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

@toddgardner This is pretty close to merge-able in my opinion. Thanks for your work on this and for dealing with my nit-picks.

We have a couple linter errors. I think the sdist error would be solved by adding the following lines on: https://github.com/burningmantech/ranger-ims-server/blob/master/MANIFEST.in

include conf/imsd-docker-compose-sample.conf
...
exclude docker-compose.yml
recursive-exclude .docker

@wsanchez let me know if you agree about that fix. I am not that familiar with the sdist setup. And if you have an idea about the other linter error: https://github.com/burningmantech/ranger-ims-server/actions/runs/8581948224/job/23520484140?pr=1163

Dockerfile Outdated Show resolved Hide resolved
MANIFEST.in Outdated
@@ -24,7 +25,9 @@ include pyproject.toml
include README.rst
include requirements/requirements*.txt
include tox.ini
recursive-include conf *.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

🤘

Copy link
Member

Choose a reason for hiding this comment

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

The sdist should only include source files, not configs used for dev testing, I think.

@chaoticbear chaoticbear self-requested a review April 11, 2024 20:43
Copy link
Contributor

@chaoticbear chaoticbear left a comment

Choose a reason for hiding this comment

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

LGTM

@wsanchez wanna take a look at this before it gets merged?

My impression is that the remaining CI issue is the same as this issue #1156 and we can ignore it for this PR

@chaoticbear
Copy link
Contributor

LGTM

@wsanchez wanna take a look at this before it gets merged?

My impression is that the remaining CI issue is the same as this issue #1156 and we can ignore it for this PR

@toddgardner can you re-base with the latest from master

@toddgardner
Copy link
Contributor Author

LGTM
@wsanchez wanna take a look at this before it gets merged?
My impression is that the remaining CI issue is the same as this issue #1156 and we can ignore it for this PR

@toddgardner can you re-base with the latest from master

Done

MANIFEST.in Outdated
@@ -1,3 +1,4 @@
exclude docker-compose.yml
Copy link
Member

Choose a reason for hiding this comment

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

Why exclude? It's part of the source repo…

Copy link
Contributor

Choose a reason for hiding this comment

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

@wsanchez I think we're unclear on what should be included. Is this a production distribution package? If so the docker-compose isn't really set up for production just yet, but could be.

Copy link
Member

Choose a reason for hiding this comment

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

No… Manifest.in is only used for the source distribution.

Comment on lines +12 to +14
*.env
/.docker/*
!/.docker/sample.env
Copy link
Member

Choose a reason for hiding this comment

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

What reads these *.env files? docker-compose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, docker-compose reads them in and uses them to populate vars in the file, some of which are passed through to the services.


# First time setup; these files are mounted into the container
cp conf/imsd-docker-compose-sample.conf conf/imsd.conf
cp conf/directory-sample.yaml conf/directory.yaml
Copy link
Member

Choose a reason for hiding this comment

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

If we are starting MySQL up we probably should also use that for the directory, but it would be a pain to set up from scratch.

We need a pair of dump files to add in this process: one for a starting directory and another for a IMS DB with something to fiddle with.

I guess that could be a follow-on PR; no need to block here.

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.05%. Comparing base (774610e) to head (8c62207).
Report is 1 commits behind head on master.

❗ Current head 8c62207 differs from pull request most recent head ee4c415. Consider uploading reports for the commit ee4c415 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   69.03%   69.05%   +0.02%     
==========================================
  Files         187      187              
  Lines        9051     9051              
  Branches     1484     1484              
==========================================
+ Hits         6248     6250       +2     
+ Misses       2702     2701       -1     
+ Partials      101      100       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wsanchez wsanchez enabled auto-merge April 23, 2024 22:51
@wsanchez wsanchez merged commit 6f11f13 into burningmantech:master Apr 23, 2024
11 of 12 checks passed
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.

3 participants