-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
regression-test: automatically fetch connection candidates #37384
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alafanechere and the rest of your teammates on Graphite |
@@ -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"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this once https://github.com/airbytehq/airbyte-platform-internal/pull/12139/files is merged
1ce942e
to
7f9ac43
Compare
There was a problem hiding this 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.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
7f9ac43
to
cf16dc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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