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

feat : Add docker-compose.yml #1043

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

Conversation

epicadk
Copy link
Member

@epicadk epicadk commented Mar 10, 2021

Description

Create docker-compose.yml and added postgresql as a service so that tests will now run on postgresql and not sqllite

Fixes #1041

Type of Change:

  • Code
  • Quality Assurance
  • Documentation

Code/Quality Assurance Only

  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Ran the commands locally on windows 10

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder
  • Update requirements.txt

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@epicadk
Copy link
Member Author

epicadk commented Mar 10, 2021

@isabelcosta should I also change github actions tests to run on docker?

@isabelcosta
Copy link
Member

is that a good practice? @epicadk is that the reasoning?

@epicadk
Copy link
Member Author

epicadk commented Mar 11, 2021

is that a good practice? @epicadk is that the reasoning?

Haha yes it solves the problem of testing the same application you ship : ).

@vj-codes
Copy link
Member

@epicadk the build is failing , can you look into that please?

@epicadk
Copy link
Member Author

epicadk commented Mar 13, 2021

@epicadk the build is failing , can you look into that please?

Yes it's because the tests are failing. I'll have to change the github actions or add some env variables, which we can discuss in the next open session.

@epicadk
Copy link
Member Author

epicadk commented Mar 29, 2021

Still trying to figure out the coverage.

@epicadk epicadk force-pushed the docker-compose branch 2 times, most recently from 50207a1 to 591ade0 Compare April 15, 2021 07:18
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #1043 (b22c754) into develop (8924616) will decrease coverage by 2.22%.
The diff coverage is 100.00%.

❗ Current head b22c754 differs from pull request most recent head d75b8b8. Consider uploading reports for the commit d75b8b8 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1043      +/-   ##
===========================================
- Coverage    94.94%   92.72%   -2.23%     
===========================================
  Files           38       38              
  Lines         2058     2021      -37     
===========================================
- Hits          1954     1874      -80     
- Misses         104      147      +43     
Impacted Files Coverage Δ
config.py 89.55% <100.00%> (+0.32%) ⬆️
app/api/dao/user.py 85.77% <0.00%> (-14.23%) ⬇️
app/api/dao/mentorship_relation.py 96.11% <0.00%> (-3.89%) ⬇️
app/database/db_types/JsonCustomType.py 73.33% <0.00%> (-3.14%) ⬇️
app/api/resources/mentorship_relation.py 96.29% <0.00%> (-0.78%) ⬇️
app/api/resources/user.py 89.75% <0.00%> (-0.62%) ⬇️
app/api/resources/admin.py 88.13% <0.00%> (-0.58%) ⬇️
app/database/models/task_comment.py 95.00% <0.00%> (-0.46%) ⬇️
run.py 96.00% <0.00%> (-0.16%) ⬇️
app/api/dao/admin.py 95.91% <0.00%> (-0.09%) ⬇️
... and 13 more

@epicadk epicadk marked this pull request as ready for review April 15, 2021 13:45
@isabelcosta isabelcosta added Status: Changes Requested Changes are required to be done by the PR author. Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Changes Requested Changes are required to be done by the PR author. labels Apr 17, 2021
Makefile Outdated
Comment on lines 16 to 20





Copy link
Member

Choose a reason for hiding this comment

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

only 1 line in the end right 😅

Makefile Show resolved Hide resolved
config.py Show resolved Hide resolved
Copy link
Member

@devkapilbansal devkapilbansal left a comment

Choose a reason for hiding this comment

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

@epicadk why we need to run tests using docker? Is there any specific reason to do so?
I have some more questions below

Dockerfile Outdated Show resolved Hide resolved
config.py Show resolved Hide resolved
docker-compose.test.yml Outdated Show resolved Hide resolved
@devkapilbansal devkapilbansal added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels May 5, 2021
@epicadk
Copy link
Member Author

epicadk commented May 5, 2021

@epicadk why we need to run tests using docker? Is there any specific reason to do so?
I have some more questions below

Because we want to run the tests on postgres. Made the requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: Create file for docker compose
4 participants