Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Move root-level docker files into docker/ subdir #877

Merged
merged 19 commits into from
Jul 16, 2022
Merged

Move root-level docker files into docker/ subdir #877

merged 19 commits into from
Jul 16, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jul 14, 2022 •
edited by sanders41
Loading

Purpose

I thought there were too many files at the root level. I also saw some opportunities for refactoring a few of the docker-related files

Changes

  • merge Dockerfile.app and Dockerfile.worker into a single Dockerfile with different build targets for each purpose (they overlapped 99%)
  • remove the docker-compose.no-db.yml file since the same thing can be accomplished using existing docker compose command line options
  • the same as above ^ but for docker-compose.worker.yml
  • move all of the .py scripts at the root level into scripts/
  • update all of the places that reference scripts to look in scripts/
  • update scripts/run_infrastructure.py to use the python sleep instead of the shell sleep (windows interop)
  • update the Makefile commands to be aware of the changes

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #

@ThomasLaPiana
Copy link
Contributor Author

@seanpreston Dockerfile.worker and Dockerfile.app seem to be practically identical, am I overlooking something? Otherwise I think they could be a single file and leverage build stages for the slight variation?

@ThomasLaPiana
Copy link
Contributor Author

@seanpreston Dockerfile.worker and Dockerfile.app seem to be practically identical, am I overlooking something? Otherwise I think they could be a single file and leverage build stages for the slight variation?

nevermind I already compared by eye and did it, all seems working 🤷

@ThomasLaPiana
Copy link
Contributor Author

@conceptualshark can you help me update the docs for this change? thank you! 🙏

@ThomasLaPiana
Copy link
Contributor Author

mssql, mysql, maria and mongo all have tests that are failing. Trying to figure out what I did to break those

@ThomasLaPiana
Copy link
Contributor Author

sidenote that it takes a really long time to run tests, which definitely slows down dev time here

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review July 15, 2022 05:41
@ThomasLaPiana
Copy link
Contributor Author

@sanders41 this is ready for a review, I figure you're a good choice for this since its a lot of Docker/infra changes

sanders41
sanders41 reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@sanders41 sanders41 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good find on the volume issue @ThomasLaPiana

@seanpreston once we have this ready do you want us to wait to merge until after the release today?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
docker/docker-compose.integration-mongodb.yml Outdated Show resolved Hide resolved
fidesops.toml Outdated Show resolved Hide resolved
scripts/run_infrastructure.py Outdated Show resolved Hide resolved
tests/scripts/test_create_superuser.py Show resolved Hide resolved
conceptualshark
conceptualshark reviewed Jul 15, 2022
View reviewed changes
Copy link
Contributor

@conceptualshark conceptualshark left a comment

Choose a reason for hiding this comment

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

I think most of the way these changes are referenced in the docs should stay the same - they're either referencing docker compose up or various make commands which haven't changed at that level. Did a brief sanity check making sure the commonly referenced commands do what they should and it looks good to me.

Is there anything else you think is missing?

ThomasLaPiana and others added 2 commits July 15, 2022 08:52
@ThomasLaPiana @sanders41
Apply suggestions from code review
8d6e88f 
Co-authored-by: Paul Sanders <[email protected]>
@ThomasLaPiana @sanders41
Update CHANGELOG.md
004fdd5 
Co-authored-by: Paul Sanders <[email protected]>
@ThomasLaPiana
Copy link
Contributor Author

I think most of the way these changes are referenced in the docs should stay the same - they're either referencing docker compose up or various make commands which haven't changed at that level. Did a brief sanity check making sure the commonly referenced commands do what they should and it looks good to me.

Is there anything else you think is missing?

nope! Just wanted to confirm I wasn't missing anything, thank you!

@ThomasLaPiana
Copy link
Contributor Author

@sanders41 ready for another check

sanders41
sanders41 approved these changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

lgtm

We'll just wait to merge until after today release goes out.

@sanders41 sanders41 merged commit bb4ea2f into main Jul 16, 2022
@sanders41 sanders41 deleted the ThomasLaPiana-cleanup-root branch July 16, 2022 02:34
sanders41 added a commit that referenced this pull request Sep 22, 2022
@ThomasLaPiana @sanders41
Move root-level docker files into docker/ subdir (#877)
854de6b 
* Move root-level docker files into docker/ subdir

* move all of the compose files

* remove root aux compose files, refactor away the no-db and worker compose files

* unify the app and worker dockerfiles into a single file that leverages build stages

* move python scripts into a subdir, fix paths in compose integration files

* fix the script tests

* use python sleep instead of system sleep, fix script paths

* remove the analytics_id that accicentally got committed

* updated changelog

* move the sample sql data to a subdir of docker/ so it can be mounted

* update the teardown command and fix the integration files

* fix more path typos

* more desperate tweaks

* fix mysql/mariadb/mongo tests

* added an additional build step if mssql not there, all tests passing

* fix an accidental lowercasing

* Apply suggestions from code review

Co-authored-by: Paul Sanders <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Paul Sanders <[email protected]>

Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: Paul Sanders <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@conceptualshark conceptualshark conceptualshark left review comments

@sanders41 sanders41 sanders41 approved these changes

@seanpreston seanpreston Awaiting requested review from seanpreston

@TheAndrewJackson TheAndrewJackson Awaiting requested review from TheAndrewJackson

Assignees

@ThomasLaPiana ThomasLaPiana

@conceptualshark conceptualshark

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ThomasLaPiana @sanders41 @conceptualshark