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

New Dockerfile #4487

Merged
merged 28 commits into from
Dec 22, 2021
Merged

New Dockerfile #4487

merged 28 commits into from
Dec 22, 2021

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented Dec 14, 2021

resolves - Partially resolves #4495

Description

Updated dockerfile. Images created should retain the previous dockerfile's functionality without requiring all the scripting and env var juggling.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@iknox-fa
Copy link
Contributor Author

@leahwicz do we have an issue for this? My github-issue-searching-fu is failing me if we do.

@iknox-fa iknox-fa changed the title new docker setup New Dockerfile Dec 14, 2021
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Nice work! I left a few comments/questions.

I don't think we ever opened a new ticket for this, after closing #3968 (comment). Let's definitely do that. The ticket should also include, incorporating image builds into the release process (GitHub Action) for dbt-core and each adapter plugin.

docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

This dockerfile looks good and works as I expected. I left some questions for you, but it works at the end of the day. We lost the ability for reproducible builds by using pip and letting it resolve dependencies, but I actually think that is fine!

When it comes to automating the build process with GHA, I think it will be a lot easier if each database adapter repository had an individual Dockerfile responsible for its own docker image. I think dbt-core should be database agnostic as possible and that includes build processes. Automated processes for database specific python packages live in the repository that houses that database adapter, I think we could relatively easily hook into that build process for building docker images at release time.

That said, we can build images and publish them for 1.0 today with this merged manually and I don't think ^^ should block that!

docker/README.md Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
@iknox-fa
Copy link
Contributor Author

This dockerfile looks good and works as I expected. I left some questions for you, but it works at the end of the day. We lost the ability for reproducible builds by using pip and letting it resolve dependencies, but I actually think that is fine!

Given that part of what we get from reproducible builds is an added level of personal safety* I'd like to unpack this a little further with the team at some point

* a fact I never considered until I did some reading on repro builds this morning.

When it comes to automating the build process with GHA, I think it will be a lot easier if each database adapter repository had an individual Dockerfile responsible for its own docker image. I think dbt-core should be database agnostic as possible and that includes build processes. Automated processes for database specific python packages live in the repository that houses that database adapter, I think we could relatively easily hook into that build process for building docker images at release time.

I was imagining the opposite paradigm where the dockerfile might move out of all the current repos and lives on it's own in a separate repo and supports all the docker images we need to build. This kind of falls into best practices of separation of concerns and DRY IMO. Can you say a bit more about why multiple dockers is easier?

@iknox-fa iknox-fa merged commit 742cd99 into main Dec 22, 2021
@iknox-fa iknox-fa deleted the update-dockerfile branch December 22, 2021 14:29
iknox-fa added a commit that referenced this pull request Feb 8, 2022
New Dockerfile supporting individual db adapters and architectures

automatic commit by git-black, original commits:
  742cd99
@dbeatty10 dbeatty10 mentioned this pull request Feb 21, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Update Dockerfile and docker image release process
3 participants