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 Iceberg: Fix tests to run in airbyte-ci #45206

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

gisripa
Copy link
Contributor

@gisripa gisripa commented Sep 6, 2024

What

Fixing tests, reconfiguring docker ports to run in airbyte-ci environment.

How

Review guide

  • Removed docker-compose dependency for bootstrapping rest catalog containers. Converted them to individual docker containers
  • Disabled Hive related tests and renamed its dependency to MinioContainerToDelete.
  • New MinioContainer for reusing the dependency in other catalogs.
  • Quick fixes in config for host-port to use container network vs host network depending on connector run calls vs test runner calls.

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Sep 6, 2024

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 Sep 17, 2024 2:56am

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/destination/iceberg labels Sep 6, 2024
Copy link
Contributor Author

gisripa commented Sep 6, 2024

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

Join @gisripa and the rest of your teammates on Graphite Graphite

@gisripa gisripa force-pushed the gireesh/09-06-iceberg-fix-tests branch 3 times, most recently from b73fdbb to 1c4c687 Compare September 16, 2024 17:11
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Sep 16, 2024
@gisripa gisripa force-pushed the gireesh/09-06-iceberg-fix-tests branch from 1c4c687 to 57417ac Compare September 16, 2024 18:30
@gisripa gisripa force-pushed the gireesh/09-06-iceberg-fix-tests branch from 57417ac to 3aa0d8a Compare September 16, 2024 18:36
@gisripa gisripa changed the base branch from master to cdk-changes-iceberg September 16, 2024 18:36
@octavia-squidington-iii octavia-squidington-iii removed the CDK Connector Development Kit label Sep 16, 2024
@gisripa gisripa changed the title iceberg-fix-tests Destination Iceberg: Fix tests to run in airbyte-ci Sep 16, 2024
@gisripa gisripa marked this pull request as ready for review September 16, 2024 18:41
@gisripa gisripa requested a review from a team as a code owner September 16, 2024 18:41
Base automatically changed from cdk-changes-iceberg to master September 16, 2024 18:50
@github-actions github-actions bot requested a review from a team as a code owner September 16, 2024 18:50
@gisripa gisripa force-pushed the gireesh/09-06-iceberg-fix-tests branch from 3aa0d8a to f96665c Compare September 16, 2024 20:12
@gisripa gisripa removed the request for review from a team September 16, 2024 20:14
@Override
protected String getDefaultSchema(@NotNull JsonNode config) throws Exception {
// TODO: This was NPE'ing without this return value because of Kotlin's non-null in base,
// but whats the actual value to pass instead of empty ?
Copy link
Contributor

Choose a reason for hiding this comment

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

make the function abstract?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad comment, actually was doing !! in actual tests for defaultSchema which was set as null in base class.

Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

rubberstamping

@Override
protected String getDefaultSchema(@NotNull JsonNode config) throws Exception {
// TODO: This was NPE'ing without this return value because of Kotlin's non-null in base,
// but whats the actual value to pass instead of empty ?
Copy link
Contributor

Choose a reason for hiding this comment

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

@gisripa gisripa force-pushed the gireesh/09-06-iceberg-fix-tests branch from 9e69d3d to ed9adc3 Compare September 17, 2024 02:51
@gisripa gisripa merged commit daee3b6 into master Sep 17, 2024
35 checks passed
@gisripa gisripa deleted the gireesh/09-06-iceberg-fix-tests branch September 17, 2024 14:53
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/destination/iceberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants