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

move basic normalization to connectors_base build #7068

Merged
merged 11 commits into from
Oct 16, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Oct 15, 2021

Closes #7069

What

  • The platform depends on a pinned version of normalization so rebuilding every time we build the platform doesn't add any better build guarantees.
  • The platform should just treat normalization like it would any other pinned artifact.

How

  • Only build basic normalization as part of the CONNECTORS_BASE build.
  • Change how normalization publishes multiple images to happen as part of the build for base-normalization.
  • Update the manage.sh script to handle pushing multiple images for normalization (it is the only "connector", for now, that publishes multiple images)
  • Update the check_image_exists.sh script to make sure that the normalization images are actually available on dockerhub (we actually weren't doing this at all before 😅 )

Tradeoff

  • The one tradeoff here is that if you are developing on normalization and for some reason you need to run the platform with the dev version, you need to remember to rebuild it manually because it won't happen as part of composeBuild. This is a pretty low cost be cause 1. the integration tests in normalization should be where we are running tests 2. You already have to manually update the normalization version to dev in order to run against a dev version of normalization, so you're already doing a manual process anyway.

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2021

CLA assistant check
All committers have signed the CLA.

@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 03:05 Inactive
Comment on lines -53 to -80
normalization:
image: airbyte/normalization:${VERSION}
build:
dockerfile: Dockerfile
context: airbyte-integrations/bases/base-normalization
labels:
io.airbyte.git-revision: ${GIT_REVISION}
normalization-mssql:
image: airbyte/normalization-mssql:${VERSION}
build:
dockerfile: mssql.Dockerfile
context: airbyte-integrations/bases/base-normalization
labels:
io.airbyte.git-revision: ${GIT_REVISION}
normalization-mysql:
image: airbyte/normalization-mysql:${VERSION}
build:
dockerfile: mysql.Dockerfile
context: airbyte-integrations/bases/base-normalization
labels:
io.airbyte.git-revision: ${GIT_REVISION}
normalization-oracle:
image: airbyte/normalization-oracle:${VERSION}
build:
dockerfile: oracle.Dockerfile
context: airbyte-integrations/bases/base-normalization
labels:
io.airbyte.git-revision: ${GIT_REVISION}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristopheDuong why were these added here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is not possible to “edit” and choose what normalization version you want like you can with connectors, I switched normalization version tags to follow airbyte core versions instead as discussed in this thread (I also discussed it with people separately and overall people were supportive of the switch) https://airbytehq.slack.com/archives/C019WEENQRM/p1633710746415900

See also comment making it part of composeBuild: #2054 (comment)

Overall it’d make it easier to publish new normalization images (when releasing airbyte) instead of doing it whenever a PR is merged…. (the docker image is not useable until there is a release of airbyte anyway)

And now that we have multiple docker images for normalization to publish, we'd have to include them in the docker-compose-build.yaml to make them as part of airbyte release?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could revert back but then we need to adapt the publish script and the DefaultNormalizationRunnerFactory too

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a good way forward is to split normalisation out to use it's own versioning and inject this version by env var.

After this we split the builds so normalisation is a separate build from the platform. In this sense, we have 3 build paths 1) connectors 2) normalisation 3) platform. This will allow the platform team to remove Python from our/their deps.

To simplify things, we can continue to release normalisation with the current OSS release. We can also continue to pin to the Airbyte version. There is some unnecessary work, but it's mostly automated so no sweat off our backs.

We can also start with a single global normalisation version. We can add individual connector normalisation versions as we run into those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I'm reverting back to how we had it that normalization has its own version that is de coupled from platform. I think the next thing to figure out is how we want to package normalization in the future. It seems like either packaging mysql normalization in the mysql destination container or at least having that container specify the name of the container that should be used to normalize it seem like 2 reasonable paths forward but those are just idea, open to others.

@cgardens cgardens marked this pull request as ready for review October 15, 2021 03:07
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 03:21 Inactive
@ChristopheDuong
Copy link
Contributor

the platform depends on a pinned version of normalization so rebuilding every time we build the platform doesn't add any better build guarantees.
The platform should just treat normalization like it would any other pinned artifact.

That is not true anymore, see this issue: #2054

@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 17:05 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Oct 15, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346818749
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346818749

@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 17:10 Inactive
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 17:31 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Oct 15, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346897430
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346897430

@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 17:36 Inactive
@github-actions github-actions bot added the area/worker Related to worker label Oct 15, 2021
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 17:42 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Oct 15, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346940671
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346940671

@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 17:50 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 17:51 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Oct 15, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346975505
❌ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1346975505

@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 18:03 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 18:04 Inactive
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 18:23 Inactive
@cgardens
Copy link
Contributor Author

cgardens commented Oct 15, 2021

/publish connector=bases/base-normalization

🕑 bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1347048248
✅ bases/base-normalization https://github.com/airbytehq/airbyte/actions/runs/1347048248

@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 18:25 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets October 15, 2021 18:26 Inactive
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 19:12 Inactive
@cgardens cgardens temporarily deployed to more-secrets October 15, 2021 20:30 Inactive
@cgardens cgardens requested a review from jrhizor October 16, 2021 00:04
@cgardens
Copy link
Contributor Author

@ChristopheDuong I updated this PR to handle everything we discussed today.

@cgardens cgardens temporarily deployed to more-secrets October 16, 2021 00:15 Inactive
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

Nice! I like that we didn't sacrifice the convenience of publishing multiple images with one command. The manage.sh script is becoming kind of unwieldy though, but nothing super pressing

@cgardens cgardens merged commit f194f35 into master Oct 16, 2021
@cgardens cgardens deleted the cgardens/move_normalization_to_conn_build branch October 16, 2021 00:26
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
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.

Remove normalization from platform build
6 participants