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

destination-s3: Use airbyte/java-connector-base:1.0.0 #50960

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alafanechere
Copy link
Contributor

Make the connector use our official base image for java connectors

@alafanechere alafanechere requested a review from a team as a code owner January 7, 2025 14:17
Copy link

vercel bot commented Jan 7, 2025

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

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 8, 2025 4:07pm

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@@ -2,12 +2,14 @@ data:
connectorSubtype: file
connectorType: destination
definitionId: 4816b78f-1489-44c1-9060-4b19d5fa9362
dockerImageTag: 1.5.0-rc.4
dockerImageTag: 1.5.0-rc.5
Copy link
Contributor

Choose a reason for hiding this comment

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

The version should be 1.5.1. And progressive rollout should be disabled. Unless you think there's risk to not rolling this out slowly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnny-schmidt I think we can indeed not use progressive rollout for this release. But your team should coordinate to make sure we don't merge this before 1.5.0 is out - as it'll otherwise stop the progressive rollout of 1.5.0-rc.4. Wdyt?

Copy link
Contributor

@johnny-schmidt johnny-schmidt Jan 9, 2025

Choose a reason for hiding this comment

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

Actually I think the right way to proceed here is just to let you merge this as rc.5, then I will just fail that rollout, and release rc.6 on top of you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually looks like rc.5 got in ahead of you, you'll need to bump to rc.6, I'll take care of rolling the release forward.

Copy link
Contributor

@johnny-schmidt johnny-schmidt left a comment

Choose a reason for hiding this comment

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

This can't be merged until the progressive rollout in progress is completed.

@johnny-schmidt
Copy link
Contributor

@alafanechere what's the level of risk here? What exactly is different about the test image. Have you verified that this works in cloud / do you need one of us to perform that test?

@alafanechere
Copy link
Contributor Author

@alafanechere what's the level of risk here? What exactly is different about the test image. Have you verified that this works in cloud / do you need one of us to perform that test?

@johnny-schmidt the risk is very low. A successful build is enough IMO to consider it stable. We just changed the build logic - from a base image on which we pre-bundle some system deps / entrypoint. But we did not change the system at all. We already shipped this change smoothly on other java connectors so I feel pretty confident.

Copy link
Contributor

@johnny-schmidt johnny-schmidt left a comment

Choose a reason for hiding this comment

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

Instead of waiting for the current S3 release to complete, we will just add this to it by

  • release this as rc.5
  • i will fail rc.5
  • release rc.6 with these changes included

@johnny-schmidt
Copy link
Contributor

Since I need to move the release forward, I'm going to go ahead and turn on automerge for this.

@johnny-schmidt johnny-schmidt enabled auto-merge (squash) January 9, 2025 17:41
@alafanechere
Copy link
Contributor Author

@johnny-schmidt feel free to orchestrate the versioning and merge 👍

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 base-image-migration connectors/destination/s3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants