Bmoric/remove docker compose for build #7500

benmoriceau · 2021-10-29T21:27:08Z

What

This making the build using a gradle plugin instead of using docker-compose build.
It aims to make the build to be more incremental as described in #7306

Building the docker image don't rely on docker-compose anymore.
The docker build step is isolated into a dedicated folder (in order to make sure that gradle plugin don't recompute the build of the docker container)
Gradle is responsible for copying the files that docker needs to build its image.
That removes the need of having a dockerignore file.

This might not be effective until #7539 is solved.

benmoriceau · 2021-10-29T21:38:46Z

I am not sure that it will be an improvment probably because of

Execution optimizations have been disabled for task ':airbyte-workers:jar' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/Users/benoitmoriceau/workspace/airbyte/airbyte-workers/build/libs/io.airbyte-airbyte-workers-0.30.23-alpha.jar'. Reason: Task ':airbyte-workers:buildMyAppImage' uses this output of task ':airbyte-workers:jar' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

I move back the review to a draft until I fix this issue.

cgardens · 2021-11-01T17:30:14Z

airbyte-scheduler/app/build.gradle

+def buildTag = System.getenv('VERSION') ?: 'dev'
+// Use task types
+task buildMyAppImage(type: DockerBuildImage) {
+    dependsOn processResources


why does building the image depend on processTestResources, compileTestJava, test, jacocoTestReport? why wouldn't it just depend on what is required to create the artifacts. like, I'm wondering if it could just depend on assemble.

i'm asking this based on my understanding of the task dependency structure of the gradle java plugin: https://docs.gradle.org/current/userguide/java_plugin.html

The plugin I am using has some implicit dependencies that will make the execution optimization to be ignore if the dependencies are not explicitly written. Like:

- Gradle detected a problem with the following location: '/Users/benoitmoriceau/workspace/airbyte/airbyte-db/lib/build/reports/jacoco/test/html'. Reason: Task ':airbyte-db:lib:buildMyAppImage' uses this output of task ':airbyte-db:lib:jacocoTestReport' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

I will try to only depend on build.

@cgardens I tried to make it to depends only on jar and some optimization have gone away with because of that:

Execution optimizations have been disabled for task ':airbyte-scheduler:app:startScripts' to ensure correctness due to the following reasons: - Gradle detected a problem with the following location: '/Users/benoitmoriceau/workspace/airbyte/airbyte-scheduler/app/build/scripts'. Reason: Task ':airbyte-scheduler:app:buildMyAppImage' uses this output of task ':airbyte-scheduler:app:startScripts' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.

…ove-docker-compose-for-build

… container

benmoriceau · 2021-11-03T23:09:53Z

I re-request a review since there were quite a lot of changes since the latest approval.

jrhizor

Looks good. Only minor comments and the build errors need to be resolved ofc.

jrhizor · 2021-11-03T23:53:31Z

docs/contributing-to-airbyte/developing-on-docker.md

+
+## Version
+
+This doc is relevant starting from the version 0.30.28-alpha. Prior to that, it was another design.


Can remove this section since nobody cares about the prior design.

Ok, That's surprising I would have expect something like Since in the javadoc,

jrhizor · 2021-11-03T23:55:34Z

docs/contributing-to-airbyte/developing-on-docker.md

@@ -0,0 +1,46 @@
+# Developing on docker


This doc needs to be added to SUMMARY.md so it actually shows up.

jrhizor · 2021-11-03T23:56:36Z

docs/contributing-to-airbyte/developing-on-docker.md

+
+## Incrementality 
+
+The docker build is fully incremental, which means that it will only build an image if it is needed. We need to keep it that way.


It's worth clarifying that this is for the platform build.

benmoriceau · 2021-11-04T00:16:09Z

Looks good. Only minor comments and the build errors need to be resolved ofc.

@jrhizor, the comments are addressed, the failing test for the build is failing in the action but not on my local.

…ove-docker-compose-for-build

This reverts commit 4e17fa2.

This reverts commit 797d11a.

…" (#7746) This reverts commit 797d11a. Restore the removal of the docker compose file

This making the build using a gradle plugin instead of using docker-compose build. It aims to make the build to be more incremental as described in airbytehq#7306 Building the docker image don't rely on docker-compose anymore. The docker build step is isolated into a dedicated folder (in order to make sure that gradle plugin don't recompute the build of the docker container) Gradle is responsible for copying the files that docker needs to build its image. That removes the need of having a dockerignore file. This might not be effective until airbytehq#7539 is solved.

…bytehq#7698) This reverts commit 4e17fa2.

…" (airbytehq#7698)" (airbytehq#7746) This reverts commit 797d11a. Restore the removal of the docker compose file

benmoriceau added 2 commits October 29, 2021 14:21

Remove the use of docker compose to build

0f06d95

Update .dockerignore

e85f1a9

benmoriceau requested review from jrhizor and cgardens October 29, 2021 21:27

github-actions bot added area/frontend area/platform issues related to the platform area/scheduler area/server area/worker Related to worker labels Oct 29, 2021

benmoriceau added area/developer Everything related to build, developer UX build type/enhancement New feature or request labels Oct 29, 2021

benmoriceau marked this pull request as draft October 29, 2021 21:34

Add explicit dependencies

dbbb298

benmoriceau temporarily deployed to more-secrets October 29, 2021 22:08 Inactive

benmoriceau temporarily deployed to more-secrets October 29, 2021 22:31 Inactive

benmoriceau added 4 commits October 29, 2021 16:30

Allign .dockerfileingnore, rm implicit dependencies

9b00dc5

improve scheduler gradle

0f049e7

Clean up migration gradle file

8957cd7

Clean up server gradle

e3dd246

benmoriceau temporarily deployed to more-secrets November 1, 2021 17:11 Inactive

cgardens reviewed Nov 1, 2021

View reviewed changes

benmoriceau added 3 commits November 1, 2021 11:01

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/rem…

8f83ea8

…ove-docker-compose-for-build

tmp

5ab190c

Look at diff

7d32b90

benmoriceau temporarily deployed to more-secrets November 1, 2021 20:58 Inactive

rm unwanted files

d693c5c

benmoriceau temporarily deployed to more-secrets November 1, 2021 21:00 Inactive

Make sure that we don't commit file that will be copied to the docker…

8084031

… container

benmoriceau temporarily deployed to more-secrets November 3, 2021 23:07 Inactive

Update docs and remove unused import

d64750a

benmoriceau temporarily deployed to more-secrets November 3, 2021 23:28 Inactive

Fix build and remove unused pluggins

d42857a

benmoriceau temporarily deployed to more-secrets November 3, 2021 23:49 Inactive

jrhizor approved these changes Nov 3, 2021

View reviewed changes

Update doc based on PR comments

24fd33c

benmoriceau temporarily deployed to more-secrets November 4, 2021 00:15 Inactive

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/rem…

2bf9b18

…ove-docker-compose-for-build

benmoriceau temporarily deployed to more-secrets November 4, 2021 17:07 Inactive

benmoriceau temporarily deployed to more-secrets November 4, 2021 20:57 Inactive

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/rem…

0a378cc

…ove-docker-compose-for-build

benmoriceau temporarily deployed to more-secrets November 5, 2021 15:31 Inactive

format

1d3d22f

benmoriceau temporarily deployed to more-secrets November 5, 2021 18:27 Inactive

benmoriceau merged commit 4e17fa2 into master Nov 5, 2021

benmoriceau deleted the bmoric/remove-docker-compose-for-build branch November 5, 2021 18:58

h7kanna mentioned this pull request Nov 5, 2021

Fixed Mac M1 build #7687

Merged

benmoriceau added a commit that referenced this pull request Nov 6, 2021

Revert "Bmoric/remove docker compose for build (#7500)"

1dcbdb0

This reverts commit 4e17fa2.

benmoriceau added a commit that referenced this pull request Nov 6, 2021

Revert "Bmoric/remove docker compose for build (#7500)" (#7698)

797d11a

This reverts commit 4e17fa2.

jrhizor mentioned this pull request Nov 6, 2021

Bump Airbyte version from 0.30.31-alpha to 0.30.32-alpha #7700

Merged

benmoriceau added a commit that referenced this pull request Nov 8, 2021

Revert "Revert "Bmoric/remove docker compose for build (#7500)" (#7698)"

32d1627

This reverts commit 797d11a.

benmoriceau added a commit that referenced this pull request Nov 8, 2021

Revert "Revert "Bmoric/remove docker compose for build (#7500)" (#7698)…

42b378f

…" (#7746) This reverts commit 797d11a. Restore the removal of the docker compose file

jrhizor mentioned this pull request Nov 8, 2021

Bump Airbyte version from 0.30.34-alpha to 0.30.35-alpha #7763

Merged

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

Revert "Bmoric/remove docker compose for build (airbytehq#7500)" (air…

554e508

…bytehq#7698) This reverts commit 4e17fa2.

schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022

Revert "Revert "Bmoric/remove docker compose for build (airbytehq#7500)…

9ddda23

…" (airbytehq#7698)" (airbytehq#7746) This reverts commit 797d11a. Restore the removal of the docker compose file

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bmoric/remove docker compose for build #7500

Bmoric/remove docker compose for build #7500

benmoriceau commented Oct 29, 2021 •

edited

Loading

benmoriceau commented Oct 29, 2021

cgardens Nov 1, 2021

cgardens Nov 1, 2021

benmoriceau Nov 1, 2021

benmoriceau Nov 1, 2021 •

edited

Loading

benmoriceau commented Nov 3, 2021

jrhizor left a comment

jrhizor Nov 3, 2021

benmoriceau Nov 4, 2021

jrhizor Nov 3, 2021

benmoriceau Nov 4, 2021

jrhizor Nov 3, 2021

benmoriceau Nov 4, 2021

benmoriceau commented Nov 4, 2021


		## Version

		This doc is relevant starting from the version 0.30.28-alpha. Prior to that, it was another design.


		## Incrementality

		The docker build is fully incremental, which means that it will only build an image if it is needed. We need to keep it that way.

Bmoric/remove docker compose for build #7500

Bmoric/remove docker compose for build #7500

Conversation

benmoriceau commented Oct 29, 2021 • edited Loading

What

benmoriceau commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

benmoriceau commented Nov 3, 2021

jrhizor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau commented Nov 4, 2021

benmoriceau commented Oct 29, 2021 •

edited

Loading

benmoriceau Nov 1, 2021 •

edited

Loading