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

Clean up CI config and Docker image #122

Merged
merged 36 commits into from
Mar 4, 2021
Merged

Clean up CI config and Docker image #122

merged 36 commits into from
Mar 4, 2021

Conversation

dataders
Copy link
Collaborator

@dataders dataders commented Mar 2, 2021

  • CircleCI config is now:
    • greatly cleaned up, and
    • installed the package and deps inline and no longer conflicts with version in Docker container.
  • Docker image for CI now:
    • no longer installs the dev_requirements.txt or dbt-sqlserver
    • uses cimg/python:3.7.10 instead of docker/python:3.7-slim as an upstream image.
    • is greatly simplfied due to reliance on CircleCI's official image

fixes #107

@dataders
Copy link
Collaborator Author

dataders commented Mar 3, 2021

@JCZuurmond I'm almost there! Just having issues installing dbt-sqlserver outside of the virtual environment. I think I need to explicitly install and upgrade pip inside the docker image...

Copy link
Contributor

@JCZuurmond JCZuurmond 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 the struggles with installing dbt-sqlserver occur because python is installed in the docker image. I think moving the python part out of the docker file should solve this

.circleci/Dockerfile Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@dataders dataders left a comment

Choose a reason for hiding this comment

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

@JCZuurmond ok now things are looking much cleaner IMHO. what do you think?

.circleci/Dockerfile Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@dataders dataders requested a review from JCZuurmond March 3, 2021 09:48
@dataders dataders changed the title Minimize docker image; cache dependencies Clean up CI config and Docker image Mar 3, 2021
@dataders dataders marked this pull request as ready for review March 3, 2021 09:49
@dataders dataders requested a review from mikaelene March 3, 2021 09:49
@JCZuurmond
Copy link
Contributor

yes, looks good @swanderz! I think it is ready to be merged.

There is one change I suggested before, but do not mind too much if not implemented, which I think is nice: use the cimg/base image in the Dockerfile and add cimg/python as secondary image in the .circleci/config.yml. This makes it more clear in the CI which python version is expected.

Copy link
Collaborator

@mikaelene mikaelene left a comment

Choose a reason for hiding this comment

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

Looks great!

@dataders dataders merged commit 51ce26e into master Mar 4, 2021
@sdebruyn sdebruyn deleted the minimize-docker-image branch June 4, 2022 08:37
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.

Remove pyodbc docker file
3 participants