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

regression-test: automatically fetch connection candidates #37384

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Apr 17, 2024

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/7070
Closes https://github.com/airbytehq/airbyte-internal-issues/issues/6598

This PR makes the regression test tool able to suggest users which connection to use for testing given a source connector. The connection id is not longer a required CLI input.

It depends on https://github.com/airbytehq/airbyte-platform-internal/pull/12139 to be merged

Copy link

vercel bot commented Apr 17, 2024

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 Apr 25, 2024 1:14pm

Copy link
Contributor Author

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

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

@alafanechere alafanechere marked this pull request as ready for review April 17, 2024 15:58
@alafanechere alafanechere requested a review from a team as a code owner April 17, 2024 15:58
@@ -27,7 +27,7 @@ pydash = "~=7.0.7"
docker = ">=6,<7"
asyncclick = "^8.1.7.1"
# TODO: when this is open-sourced, don't require connection-retriever
connection-retriever = {git = "git@github.com:airbytehq/airbyte-platform-internal", subdirectory = "tools/connection-retriever"}
connection-retriever = {git = "git@github.com:airbytehq/airbyte-platform-internal", subdirectory = "tools/connection-retriever", branch = "augustin/04-16-connection_retriever_automatically_fetch_connections_for_testing"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alafanechere alafanechere force-pushed the augustin/04-17-regression-test_automatically_fetch_connection_candidates branch from 1ce942e to 7f9ac43 Compare April 18, 2024 13:31
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

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

Thanks for adding stream selection! I just have a couple of small requests/questions left.

Saw your comments on my PR for optional connection ID - I'll stack it on top of this.

Comment on lines +134 to +140
if source_docker_image := config.stash[stash_keys.CONNECTION_OBJECTS].source_docker_image:
config.stash[stash_keys.CONTROL_VERSION] = source_docker_image.split(":")[-1]
else:
config.stash[stash_keys.CONTROL_VERSION] = "latest"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning is:

  • If you passed custom config/state/catalog we are not calling the DB and have no clue about which is the current production version of a connector, so we fallback to latest as the control version
  • If we retrieve the connection from the DB we can have an accurate control version in case the connector version was pinned in production to a non latest version.

connection_id, retrieved_objects = retrieve_objects(
requested_objects,
retrieval_reason=retrieval_reason,
connection_id=connection_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this should also include selected_streams.

if connection_id is None and not auto_select_connection:
raise ValueError("A connection id or auto_select_connection must be provided to retrieve the connection objects.")
if auto_select_connection and not connector_image:
raise ValueError("A connector image must be provided when using auto_select_connection.")

custom_config = get_connector_config_from_path(custom_config_path) if custom_config_path else None
custom_configured_catalog = get_configured_catalog_from_path(custom_configured_catalog_path) if custom_configured_catalog_path else None
Copy link
Contributor

Choose a reason for hiding this comment

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

WDTY about also applying selected_streams to the custom configured catalog, so that a user can pass in a catalog and doesn't have to edit it in order to test only the selected streams?

@alafanechere alafanechere force-pushed the augustin/04-17-regression-test_automatically_fetch_connection_candidates branch from 7f9ac43 to cf16dc2 Compare April 25, 2024 13:14
Copy link
Contributor

@clnoll clnoll left a comment

Choose a reason for hiding this comment

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

LGTM!

@alafanechere alafanechere merged commit e172376 into master Apr 25, 2024
34 checks passed
@alafanechere alafanechere deleted the augustin/04-17-regression-test_automatically_fetch_connection_candidates branch April 25, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants