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

fix acceptance tests deps #4939

Merged
merged 3 commits into from
Jul 23, 2021
Merged

Conversation

cgardens
Copy link
Contributor

What

How

  • remove the deps that were added. this requires...
  • removing an unused dep declared in gradle on postgres source and destination
  • having acceptance tests rely on a pinned version of source-e2e-test or destination-e2e-test

Recommended reading order

  1. x.java
  2. y.python

@github-actions github-actions bot added the area/connectors Connector related issues label Jul 23, 2021
@cgardens cgardens requested a review from davinchia July 23, 2021 04:48
@@ -247,18 +247,8 @@ jobs:
# - name: Get Docker Space
# run: docker run --rm busybox df -h

- name: Image Cleanup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these should not be needed anymore with the slimmer build.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch. should we also remove the script? or keep just in case?

# make sure these always run before pushing platform docker images
- name: Run End-to-End Acceptance Tests
if: success() && github.ref == 'refs/heads/master'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we run k8s acceptance tests as part of PR. no reason we shouldn't do it for docker. also doing so would have cost the dependencies i missed in my PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

definitely. surprised this aren't running always.

@@ -61,12 +61,6 @@ task acceptanceTests(type: Test) {
exceptionFormat "full"
}
mustRunAfter test

dependsOn ':airbyte-integrations:connectors:source-postgres:airbyteDocker'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these should not have been here anymore. we use pinned versions of postgres source and destination

dependsOn ':airbyte-integrations:connectors:source-postgres:airbyteDocker'
dependsOn ':airbyte-integrations:connectors:destination-postgres:airbyteDocker'
// for checkpointing tests
dependsOn ':airbyte-integrations:connectors:source-e2e-test:airbyteDocker'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

switching to pinned versions of these makes these unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@@ -97,7 +97,7 @@ public void testAutomaticMigration()
logs.remove(keyToRemove);
}

LOGGER.info(log);
LOGGER.info(log.trim());
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 helm with extra whitespace that gets added.

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Thanks Charles!

@cgardens cgardens merged commit c966fd7 into master Jul 23, 2021
@cgardens cgardens deleted the cgardens/fix_acceptance_tests_Deps branch July 23, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants