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

connectors-ci: fix postgres integration testing #25942

Merged
merged 19 commits into from
May 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ jobs:
CI_GIT_REVISION: ${{ github.sha }}
CI_CONTEXT: "manual"
CI_PIPELINE_START_TIMESTAMP: ${{ steps.get-start-timestamp.outputs.start-timestamp }}
S3_BUILD_CACHE_ACCESS_KEY_ID: ${{ secrets.SELF_RUNNER_AWS_ACCESS_KEY_ID }}
S3_BUILD_CACHE_SECRET_KEY: ${{ secrets.SELF_RUNNER_AWS_SECRET_ACCESS_KEY }}
- name: Run airbyte-ci connectors test [PULL REQUESTS]
if: github.event_name == 'pull_request'
run: |
Expand All @@ -88,3 +90,5 @@ jobs:
CI_GIT_REVISION: ${{ github.event.pull_request.head.sha }}
CI_CONTEXT: "pull_request"
CI_PIPELINE_START_TIMESTAMP: ${{ steps.get-start-timestamp.outputs.start-timestamp }}
S3_BUILD_CACHE_ACCESS_KEY_ID: ${{ secrets.SELF_RUNNER_AWS_ACCESS_KEY_ID }}
S3_BUILD_CACHE_SECRET_KEY: ${{ secrets.SELF_RUNNER_AWS_SECRET_ACCESS_KEY }}
2 changes: 2 additions & 0 deletions .github/workflows/connector_nightly_builds_dagger.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,5 @@ jobs:
CI_GIT_BRANCH: ${{ steps.extract_branch.outputs.branch }}
CI_CONTEXT: "nightly_builds"
CI_PIPELINE_START_TIMESTAMP: ${{ steps.get-start-timestamp.outputs.start-timestamp }}
S3_BUILD_CACHE_ACCESS_KEY_ID: ${{ secrets.SELF_RUNNER_AWS_ACCESS_KEY_ID }}
S3_BUILD_CACHE_SECRET_KEY: ${{ secrets.SELF_RUNNER_AWS_SECRET_ACCESS_KEY }}
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,11 @@
public class HostPortResolver {

public static String resolveHost(GenericContainer container) {
return System.getProperty("os.name").toLowerCase().startsWith("mac")
? getIpAddress(container)
: container.getHost();
return getIpAddress(container);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to break the clickhouse integration tests on Mac I think.
The original change was #14701 back in July last year

Copy link
Contributor

Choose a reason for hiding this comment

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

source-clickhouse that is

}

public static int resolvePort(GenericContainer container) {
return System.getProperty("os.name").toLowerCase().startsWith("mac") ? (Integer) container.getExposedPorts().get(0)
: container.getFirstMappedPort();
return (Integer) container.getExposedPorts().stream().findFirst().get();
}

public static String resolveIpAddress(GenericContainer container) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.airbyte.db.factory.DatabaseDriver;
import io.airbyte.integrations.base.ssh.SshHelpers;
import io.airbyte.integrations.standardtest.source.TestDestinationEnv;
import io.airbyte.integrations.util.HostPortResolver;
import io.airbyte.protocol.models.Field;
import io.airbyte.protocol.models.JsonSchemaType;
import io.airbyte.protocol.models.v0.CatalogHelpers;
Expand Down Expand Up @@ -60,11 +61,10 @@ protected void setupEnvironment(final TestDestinationEnv environment) throws Exc
final JsonNode replicationMethod = Jsons.jsonNode(ImmutableMap.builder()
.put("method", "Standard")
.build());
final var containerOuterAddress = SshHelpers.getOuterContainerAddress(container);
final var containerInnerAddress = SshHelpers.getInnerContainerAddress(container);

config = Jsons.jsonNode(ImmutableMap.builder()
.put("host", containerInnerAddress.left)
.put("port", containerInnerAddress.right)
.put("host", HostPortResolver.resolveHost(container))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to return the inner address and port of a container? This was the reason tests were originally failing when run on Mac.
Can you validate that running integration test on your Mac is still passing.
For example:

./gradlew --info :airbyte-integrations:connectors:source-postgres:integrationTestJava --tests "io.airbyte.integrations.io.airbyte.integration_tests.sources.PostgresSourceSSLCaCertificateAcceptanceTest"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it passed, this is why I made the change...
Screen Shot 2023-05-11 at 19 34 13

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks
I'm just making sure as I don't know dagger 😅

.put("port", HostPortResolver.resolvePort(container))
.put("database", container.getDatabaseName())
.put("schemas", Jsons.jsonNode(List.of("public")))
.put("username", "postgres")
Expand All @@ -79,8 +79,8 @@ protected void setupEnvironment(final TestDestinationEnv environment) throws Exc
config.get("password").asText(),
DatabaseDriver.POSTGRESQL.getDriverClassName(),
String.format(DatabaseDriver.POSTGRESQL.getUrlFormatString(),
containerOuterAddress.left,
containerOuterAddress.right,
container.getHost(),
container.getFirstMappedPort(),
config.get("database").asText()),
SQLDialect.POSTGRES)) {
final Database database = new Database(dslContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class CdcInitialSnapshotPostgresSourceDatatypeTest extends AbstractPostgr
private static final String SCHEMA_NAME = "test";
private static final String SLOT_NAME_BASE = "debezium_slot";
private static final String PUBLICATION = "publication";
private static final int INITIAL_WAITING_SECONDS = 5;
private static final int INITIAL_WAITING_SECONDS = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the difference on dagger expected?

Copy link
Contributor Author

@alafanechere alafanechere May 11, 2023

Choose a reason for hiding this comment

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

I faced flaky results on my local machine too and increasing the waiting time mitigate these problems on both local and dagger. I found a PR from Sergio on another connector that did it for the same reasons.


@Override
protected Database setupDatabase() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class CdcPostgresSourceAcceptanceTest extends AbstractPostgresSourceAccep
private static final String STREAM_NAME = "id_and_name";
private static final String STREAM_NAME2 = "starships";
protected static final String PUBLICATION = "publication";
protected static final int INITIAL_WAITING_SECONDS = 5;
protected static final int INITIAL_WAITING_SECONDS = 30;

protected PostgreSQLContainer<?> container;
protected JsonNode config;
Expand All @@ -63,6 +63,8 @@ protected void setupEnvironment(final TestDestinationEnv environment) throws Exc
.put("publication", PUBLICATION)
.put("initial_waiting_seconds", INITIAL_WAITING_SECONDS)
.build());


config = Jsons.jsonNode(ImmutableMap.builder()
.put(JdbcUtils.HOST_KEY, HostPortResolver.resolveHost(container))
.put(JdbcUtils.PORT_KEY, HostPortResolver.resolvePort(container))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public class CdcWalLogsPostgresSourceDatatypeTest extends AbstractPostgresSource
private static final String SCHEMA_NAME = "test";
private static final String SLOT_NAME_BASE = "debezium_slot";
private static final String PUBLICATION = "publication";
private static final int INITIAL_WAITING_SECONDS = 15;
private static final int INITIAL_WAITING_SECONDS = 30;
private JsonNode stateAfterFirstSync;

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ protected void setupEnvironment(final TestDestinationEnv environment) throws Exc

container = new PostgreSQLContainer<>("postgres:13-alpine");
container.start();

final String username = container.getUsername();
final String password = container.getPassword();
final List<String> schemas = List.of("public");
Expand Down Expand Up @@ -94,6 +95,7 @@ private JsonNode getConfig(final String username, final String password, final L
final JsonNode replicationMethod = Jsons.jsonNode(ImmutableMap.builder()
.put("method", "Standard")
.build());

return Jsons.jsonNode(ImmutableMap.builder()
.put(JdbcUtils.HOST_KEY, HostPortResolver.resolveHost(container))
.put(JdbcUtils.PORT_KEY, HostPortResolver.resolvePort(container))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public class CdcPostgresSourceTest extends CdcSourceTest {

protected static final String SLOT_NAME_BASE = "debezium_slot";
protected static final String PUBLICATION = "publication";
protected static final int INITIAL_WAITING_SECONDS = 5;
protected static final int INITIAL_WAITING_SECONDS = 30;
private PostgreSQLContainer<?> container;

protected String dbName;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class PostgresCdcGetPublicizedTablesTest {
private static final String SCHEMA_NAME = "public";
private static final String PUBLICATION = "publication_test_12";
private static final String REPLICATION_SLOT = "replication_slot_test_12";
protected static final int INITIAL_WAITING_SECONDS = 5;
protected static final int INITIAL_WAITING_SECONDS = 30;
private static PostgreSQLContainer<?> container;
private JsonNode config;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ void testLsnCommitBehaviourProperty(final String commitBehaviour) {
.put("replication_slot", "replication_slot")
.put("publication", "PUBLICATION")
.put("plugin", "pgoutput")
.put("initial_waiting_seconds", 5)
.put("initial_waiting_seconds", 30)
.put("lsn_commit_behaviour", commitBehaviour)
.build());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -408,13 +408,24 @@ def with_gradle(
.with_env_variable("VERSION", "23.0.1")
.with_exec(["sh", "-c", "curl -fsSL https://get.docker.com | sh"])
.with_exec(["mkdir", "/root/.gradle"])
.with_mounted_cache("/root/.gradle", root_gradle_cache, sharing=CacheSharingMode.LOCKED)
.with_exec(["rm", "-f", "/root/.gradle/caches/journal-1/journal-1.lock"])
.with_exec(["mkdir", "/airbyte"])
.with_mounted_directory("/airbyte", context.get_repo_dir(".", include=include))
.with_mounted_cache("/airbyte/.gradle", airbyte_gradle_cache, sharing=CacheSharingMode.LOCKED)
.with_workdir("/airbyte")
)
if context.is_ci:
Copy link
Contributor

Choose a reason for hiding this comment

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

@alafanechere Can we leave a comment around this that explains

  1. What we are doing
  2. Why
  3. When we should remove it and whos responsible

openjdk_with_docker = (
openjdk_with_docker.with_env_variable("CI", "true")
.with_secret_variable(
"S3_BUILD_CACHE_ACCESS_KEY_ID", context.dagger_client.host().env_variable("S3_BUILD_CACHE_ACCESS_KEY_ID").secret()
)
.with_secret_variable(
"S3_BUILD_CACHE_SECRET_KEY", context.dagger_client.host().env_variable("S3_BUILD_CACHE_SECRET_KEY").secret()
)
)
else:
openjdk_with_docker = openjdk_with_docker.with_mounted_cache("/root/.gradle", root_gradle_cache, sharing=CacheSharingMode.LOCKED)

if bind_to_docker_host:
return with_bound_docker_host(context, openjdk_with_docker, shared_tmp_volume, docker_service_name=docker_service_name)
else:
Expand Down