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

[airbyte-ci] Implement pre/post build hooks #31282

Merged

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Oct 11, 2023

REOPENING #30526 as I reverted it following a publish failure.
The error in publish of source-zendesk-chat was fixed by removing an await instruction here. The original error was a failing pwd execution - probably due to an architecture problem. I don't understand the root cause but I believe the early await led to a layer reuse from a different (the previously built) architecture: an amd layer used for the arm build...

I tried a pre-release of source-zendesk-chat, with build_customization, on the current branch state and it worked.

Lesson: always try a pre-release anytime a build_customization.py is added.

What

Closes #30237
In some scenario a connector might have to customize our default build process: to install a system dependency, a system env var.
To do so we want to expose a pre/post build hook mechanism that connector developers can leverage.

How

  • Developers will create a build_customization.py module in the connector code directory
  • They'll have to implement a pre_connector_install and/or post_connector_install function. They'll mutate the container object of this stage in this function.
  • Our build process will run this function: pre_connector_install is executed on top of our base image, post_connector_install is executed after the rest of all the build instructions.

@vercel
Copy link

vercel bot commented Oct 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Oct 12, 2023 6:52am

Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/zendesk-chat labels Oct 11, 2023
@alafanechere alafanechere marked this pull request as ready for review October 11, 2023 14:19
@airbyte-oss-build-runner
Copy link
Collaborator

source-zendesk-chat test report (commit a3f900b31d) - ✅

⏲️ Total pipeline duration: 01mn57s

Step Result
Build source-zendesk-chat docker image for platform(s) linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate metadata for source-zendesk-chat
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-zendesk-chat test

@airbyte-oss-build-runner
Copy link
Collaborator

source-zendesk-chat test report (commit c87cdd99f8) - ✅

⏲️ Total pipeline duration: 01mn01s

Step Result
Build source-zendesk-chat docker image for platform(s) linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate metadata for source-zendesk-chat
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-zendesk-chat test

@airbyte-oss-build-runner
Copy link
Collaborator

source-zendesk-chat test report (commit f9bdfbd9c9) - ✅

⏲️ Total pipeline duration: 01mn11s

Step Result
Build source-zendesk-chat docker image for platform(s) linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate metadata for source-zendesk-chat
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-zendesk-chat test

@alafanechere alafanechere requested a review from a team October 11, 2023 15:23
Returns:
Container: The mutated base image container.
"""
return await base_image_container.with_env_variable("MY_PRE_BUILD_ENV_VAR", "my_pre_build_env_var_value")
Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere Should we not have to remove the await here too?

Returns:
Container: The mutated base image container.
"""
return base_image_container.with_env_variable("MY_PRE_BUILD_ENV_VAR", "my_pre_build_env_var_value")
Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere I removed the await(s) in this file

We set these environment variable to match what was originally in the Dockerfile.
Disclaimer: I have no idea if these env vars are actually needed.
"""
return base_image_container.with_env_variable("AIRBYTE_IMPL_MODULE", "source_zendesk_chat").with_env_variable(
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 I understand why this would fail if we had an await here.

As I understanding calling await on a Container evaluates the container.

analogy: turns a docker file into an image

so if we awaited here we would transform the return into something we could not keep adding container commands too like with_label with_secret etc...

Now I could not figure out why it would only error when publishing a amd64 docker image and not during build.....

That is the big mystery.

I think there's enough here to approve to unblock, but if you want to keep investigating on your side I wouldnt object!

Copy link
Contributor Author

@alafanechere alafanechere Oct 12, 2023

Choose a reason for hiding this comment

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

You stated it better than I did!
I originally called await to early evaluate the hooks instruction so that we can fail fast during the build if a hook is improperly set.
await on a container evaluates it but also returns it so subsequent mutations on an awaited container work as long as you reassign it to a new variable.
I believe the problem lies in the fact that early evaluation messes something up with architecture variant... something like this container, built for a different platform in the first iteration of the build loop, is not recreated for the second platform on the second loop iteration, or the first iteration is cached...

@alafanechere alafanechere force-pushed the augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks branch from c6c64b4 to 497b614 Compare October 12, 2023 06:52
@airbyte-oss-build-runner
Copy link
Collaborator

source-zendesk-chat test report (commit 497b614e8a) - ✅

⏲️ Total pipeline duration: 02mn45s

Step Result
Build source-zendesk-chat docker image for platform(s) linux/x86_64
Unit tests
Acceptance tests
Code format checks
Validate metadata for source-zendesk-chat
Connector version semver check
Connector version increment check
QA checks

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-zendesk-chat test

@github-actions
Copy link
Contributor

Coverage report for source-postgres

There is no coverage information present for the Files changed

Total Project Coverage 71.72% 🍏

@alafanechere alafanechere merged commit 33c683b into master Oct 12, 2023
@alafanechere alafanechere deleted the augustin/09-18-_airbyte-ci_Implement_pre/post_build_hooks branch October 12, 2023 07:19
ariesgun pushed a commit to ariesgun/airbyte that referenced this pull request Oct 23, 2023
Co-authored-by: Ben Church <ben@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/zendesk-chat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support pre/post build hooks on connector build
4 participants