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

Dockerized Faculty Tools #22

Open
wants to merge 52 commits into
base: develop
Choose a base branch
from
Open

Dockerized Faculty Tools #22

wants to merge 52 commits into from

Conversation

ssilverm
Copy link
Member

@ssilverm ssilverm commented Mar 30, 2021

Proposed Changes

  • This changes Faculty Tools to use Docker and docker-compose.

Fixes #21

@ssilverm ssilverm changed the base branch from master to develop March 30, 2021 21:13
@Thetwam Thetwam added the backstage Related to improvements like tests, refactoring, continuous integration, maintenance, etc. label Mar 30, 2021
@Thetwam Thetwam linked an issue Mar 30, 2021 that may be closed by this pull request
Copy link
Member

@Thetwam Thetwam left a comment

Choose a reason for hiding this comment

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

The status checks are failing on this PR. It looks like it's a MDL configuration issue with Travis. That particular check should be easy to work around, but this PR also fails to address how "dockerizing" the application will affect CI.

Though, instead of fixing the Travis setup, we should probably be using GitHub Actions anyways (see newly-created #23)

settings.py Outdated Show resolved Hide resolved
keeeeeegan
keeeeeegan previously approved these changes Apr 2, 2021
Copy link
Member

@keeeeeegan keeeeeegan 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 well for me, though I did run into a snag getting it working because API_URL is being used as a BASE_URL in settings.py, and running this with /api/v1 added causes an error to be thrown by the consumer/Canvas that's not super clear. I'd recommend a possible variable name change or clarification in .env

keeeeeegan
keeeeeegan previously approved these changes Apr 21, 2021
Copy link
Member

@keeeeeegan keeeeeegan left a comment

Choose a reason for hiding this comment

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

LGTM

@ssilverm ssilverm requested a review from Thetwam April 18, 2022 17:44
Copy link
Member

@Thetwam Thetwam left a comment

Choose a reason for hiding this comment

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

Review: Part One (To be continued)

Need some doc updates to help avoid confusion from devs unfamiliar with the project trying to stand up an instance.

Also, I couldn't get the database to load. Error is in one of the comments below.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@ssilverm ssilverm requested a review from Thetwam April 26, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backstage Related to improvements like tests, refactoring, continuous integration, maintenance, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerize
3 participants