-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Version Normalization as if it were part of core #1208
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
plugins { | ||
id 'airbyte-docker' | ||
id 'airbyte-python' | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,15 +91,20 @@ allprojects { | |
apply plugin: 'base' | ||
|
||
afterEvaluate { project -> | ||
// format is '{project.group}.{project.name}'. | ||
// note: project.group returns a period delimited the fully qualified package name. | ||
def composeDeps = [ | ||
"airbyte-config:init", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. anything that had a |
||
"airbyte-db", | ||
"airbyte-scheduler", | ||
"airbyte-server", | ||
"airbyte-webapp", | ||
'io.airbyte.airbyte-config.init', | ||
'io.airbyte.airbyte-db', | ||
'io.airbyte.airbyte-scheduler', | ||
'io.airbyte.airbyte-server', | ||
'io.airbyte.airbyte-webapp', | ||
'io.airbyte.airbyte-integrations.bases.base-normalization' | ||
].toSet().asImmutable() | ||
|
||
if (project.name in composeDeps) { | ||
|
||
def fullProjectName = (project.group + "." + project.name).toString() | ||
if (fullProjectName in composeDeps) { | ||
composeBuild.dependsOn(project.tasks.assemble) | ||
} | ||
} | ||
|
@@ -194,7 +199,7 @@ task composeBuild { | |
doFirst { | ||
exec { | ||
workingDir rootDir | ||
commandLine 'docker-compose', '-f', 'docker-compose.build.yaml', 'build', '--parallel', '--quiet' | ||
commandLine 'docker-compose', '-f', 'docker-compose.build.yaml', '-f', 'docker-compose.normalization.build.yaml', 'build', '--parallel', '--quiet' | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
version: "3.7" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is still valuable to leverage docker-compose here to guarantee we get the same env variables as the rest of the core app. but ultimately we want the config to be separate because the normalization image is not run as part of compose up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keep in mind we will kill this normalization container at some point too and roll it into the destination, so this will hopefully be short-ish lived. but definitely around for several weeks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Normalization has the same lifecycle as the rest of core, it shouldn't be separate. I don't see Having every "Core" artifacts listed in the same place will save us a lot of headache or incomplete releases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalization does not have the same lifecycle as the rest for core. All of core is run on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we add it to docker-compose.yaml docker-compose up will fail it. It is not meant to run at that time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think michel is suggesting adding it to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed offline and agreed to keep it separate. the issue with adding to the |
||
|
||
services: | ||
normalization: | ||
image: airbyte/normalization:dev | ||
build: | ||
dockerfile: Dockerfile | ||
context: airbyte-integrations/bases/base-normalization | ||
labels: | ||
io.airbyte.git-revision: ${GIT_REVISION} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately i think this is pretty intolerable. i think it pushes me far enough to think that we should scrap this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed because now the only way that the normalization image gets built is by
composeBuild
instead of:airbyte-integrations:bases:base-normalization:airbyteDocker
. We could keep theairbyteDocker
task as well for normalization, but then we'd have 2 sources of truth for how the container gets built which also seems really terrible.