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

Version Normalization as if it were part of core #1208

Closed
wants to merge 5 commits into from

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Dec 4, 2020

Closes #1190

@@ -0,0 +1,10 @@
version: "3.7"
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 docker-compose.build.yaml as something that tells us what to run but what to build for core. So it is ok if it is declared as a service.

Having every "Core" artifacts listed in the same place will save us a lot of headache or incomplete releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 docker-compose up. Normalization is not. Normalization is run when it is invoked from the worker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think michel is suggesting adding it to the docker-compose.build.yaml file not docker-compose.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 docker-compose.build.yaml file is that when we push to docker hub to release we rely on both docker-compose.yaml AND docker-compose.build.yaml. They stack on each other, so docker-compose.yaml is part of the build / release process, but it is also how the containers get run. Agreed for now to keep it separate. But we all think it is sad. 😭

def composeDeps = [
"airbyte-config:init",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

anything that had a : wasn't working in this because project.name just returns the name no prefix (e.g. init).

Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

I would remove the new set of dockercompose files

@@ -0,0 +1,10 @@
version: "3.7"
Copy link
Contributor

Choose a reason for hiding this comment

The 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 docker-compose.build.yaml as something that tells us what to run but what to build for core. So it is ok if it is declared as a service.

Having every "Core" artifacts listed in the same place will save us a lot of headache or incomplete releases.

@@ -0,0 +1,5 @@
version: "3.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need that?

@cgardens cgardens force-pushed the cgardens/version_normalization branch from 096aeab to ad8ece3 Compare December 4, 2020 23:22
@@ -18,7 +18,8 @@ dependencies {
implementation project(':airbyte-queue')

integrationTestImplementation project(':airbyte-integrations:bases:standard-destination-test')
integrationTestImplementation files(project(':airbyte-integrations:bases:base-normalization').airbyteDocker.outputs)
// depends on normalization docker image creation, which is handled in composeBuild.
integrationTestImplementation files(task('composeBuild').outputs)
Copy link
Contributor Author

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.

Copy link
Contributor Author

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 the airbyteDocker 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.

@cgardens
Copy link
Contributor Author

cgardens commented Dec 5, 2020

I think we should pin the version. Closing this.

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.

airbyte/normalization dev version should available in dockerhub.
3 participants