Source Metabase: migrate to Beta YAML #19236

darynaishchenko · 2022-11-09T17:09:25Z

source Metabase: check list

darynaishchenko · 2022-11-09T17:13:47Z

/test connector=connectors/source-metabase

🕑 connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3430288327
❌ connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3430288327
🐛 https://gradle.com/s/xkoe5gk3a3jcu

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
=================== 1 failed, 25 passed, 2 skipped in 24.12s ===================

davydov-d · 2022-11-09T18:39:00Z

airbyte-integrations/connectors/source-metabase/acceptance-test-config.yml

+    tests:
+      - spec_path: "source_metabase/spec.yaml"
+        backward_compatibility_tests_config:
+          disable_for_version: "0.1.0"


is it still needed here? since the new version is 0.3.0 the test will be held against 0.2.0

davydov-d · 2022-11-09T18:39:31Z

airbyte-integrations/connectors/source-metabase/acceptance-test-config.yml

+      - config_path: "secrets/config.json"
+        configured_catalog_path: "integration_tests/configured_catalog.json"
+        expect_records:
+          path: "integration_tests/expected_records.txt"


what about full_refresh and incremental test cases?

davydov-d · 2022-11-09T18:43:59Z

airbyte-integrations/connectors/source-metabase/source_metabase/metabase.yaml

+    primary_key: "id"
+    retriever:
+      $ref: "*ref(definitions.retriever)"
+  data_field_base_stream:


I'd prefer removing this stream since it has a single child

davydov-d · 2022-11-09T19:20:57Z

airbyte-integrations/connectors/source-metabase/source_metabase/source.py

 API_URL = "instance_api_url"
 USERNAME = "username"
 PASSWORD = "password"
 SESSION_TOKEN = "session_token"


I believe this should have been removed

davydov-d · 2022-11-09T19:23:15Z

airbyte-integrations/connectors/source-metabase/source_metabase/source.py

            return session.has_valid_token(), None
        except Exception as e:
            return False, e
        finally:
            if session:
                session.close_session()

-    def streams(self, config: Mapping[str, Any]) -> List[Stream]:
-        self.session = MetabaseAuth(logging.getLogger("airbyte"), config)


self.close_session() will never execute since self.session is never set

davydov-d · 2022-11-09T19:25:25Z

airbyte-integrations/connectors/source-metabase/source_metabase/source.py


    def check_connection(self, logger: logging.Logger, config: Mapping[str, Any]) -> Tuple[bool, Any]:
        session = None
        try:
-            session = MetabaseAuth(logger, config)


On one hand you are defining a check in yaml with a stream passed in, on the other hand you're overriding a check method in python. This means yaml check will not work and has no sense. We have to leave just one of them, I'm in favour of yaml based check

thanks, we need custom check because of updating session token, so I left check here, and removed from yaml.

davydov-d · 2022-11-09T19:26:53Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+    def get_auth_header(self) -> Mapping[str, Any]:
+        return {"X-Metabase-Session": self.session_token}
+
+    def close_session(self):


is this ever called?

davydov-d · 2022-11-09T19:27:19Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+            raise ConnectionError(f"Failed to retrieve new session token, response code {response.status_code} because {response.reason}")
+        return self.session_token
+
+    def has_valid_token(self) -> bool:


if we leave just a yaml based check, I guess this method may be removed

davydov-d · 2022-11-09T19:29:26Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+API_URL_CONFIG_KEY = "instance_api_url"
+USERNAME_CONFIG_KEY = "username"
+PASSWORD_CONFIG_KEY = "password"
+SESSION_TOKEN_CONFIG_KEY = "session_token"


does not make much sense to me as it's not heavily reused

davydov-d · 2022-11-09T19:29:55Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+
+
+@dataclass
+class MetabaseAuth(HttpAuthenticator, JsonSchemaMixin):


please add unit tests to cover the custom component

davydov-d · 2022-11-10T08:19:34Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+
+@dataclass
+class MetabaseAuth(HttpAuthenticator, JsonSchemaMixin):
+    def __init__(self, config: Mapping[str, Any], options: Mapping[str, Any]):


One more thing: we used to have one session per sync, now we will have one session per stream. I suggest you make it a source instance attribute/cached property/etc

davydov-d · 2022-11-10T15:17:05Z

airbyte-integrations/connectors/source-metabase/source_metabase/source.py

-        ]
+        self.session = MetabaseAuth(config, {})
+
+        return super().streams(config)


Once again: you're not passing session object here. This means new session will be instantiated per each stream if an authenticator is passed to a stream in yaml. After the sync succeeds, only the source session (defined two lines above) will be closed, but not those sessions in the streams, right?

darynaishchenko · 2022-11-18T09:00:37Z

/test connector=connectors/source-metabase

🕑 connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3495534189
❌ connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3495534189
🐛 https://gradle.com/s/h2qsaz6m76veu

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - AssertionError: as...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
=================== 3 failed, 24 passed, 1 skipped in 21.97s ===================

darynaishchenko · 2022-11-18T09:42:36Z

/test connector=connectors/source-metabase

🕑 connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3495820124
❌ connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3495820124
🐛 https://gradle.com/s/plqrcd75ggpao

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - AssertionError: as...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
=================== 3 failed, 26 passed, 1 skipped in 23.09s ===================

darynaishchenko · 2022-11-18T10:41:24Z

/test connector=connectors/source-metabase

🕑 connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3496210842
❌ connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3496210842
🐛 https://gradle.com/s/7j3zarhwgvrcg

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - AssertionError: as...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contai...
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
ERROR test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - doc...
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
============== 3 failed, 25 passed, 1 skipped, 1 error in 25.24s ===============

girarda

this is great! Only request is to move the authenticator to the CDK's airbyte_cdk/sources/declarative/auth package so it can be reused. Will approve when it's done

girarda · 2022-11-18T23:57:28Z

airbyte-integrations/connectors/source-metabase/source_metabase/components.py

+
+
+@dataclass
+class MetabaseSessionTokenAuthenticator(AbstractHeaderAuthenticator, DeclarativeAuthenticator, JsonSchemaMixin):


We should be able to generalize this class and move it to the CDK. The only thing that is hardcoded is the auth_header, which we can also parameterize

done, #19716

davydov-d · 2022-11-21T13:21:24Z

airbyte-integrations/connectors/source-metabase/source_metabase/metabase.yaml

+    $options:
+      name: "dashboards"
+      path: "dashboard"
+  users_stream:


does this stream have a primary key? it's missing here

girarda · 2022-12-08T23:09:28Z

airbyte-integrations/connectors/source-metabase/unit_tests/test_component.py

@@ -0,0 +1,136 @@
+#


is this file still needed?

girarda · 2022-12-08T23:26:07Z

airbyte-integrations/connectors/source-metabase/setup.py

@@ -5,7 +5,7 @@

 from setuptools import find_packages, setup

-MAIN_REQUIREMENTS = ["airbyte-cdk~=0.2", "requests>=2.28.0", "types-requests>=2.27.30"]
+MAIN_REQUIREMENTS = ["airbyte-cdk", "requests>=2.28.0", "types-requests>=2.27.30", "cachetools"]


we shouldn't need this dependency here since it'll be included in the cdk as part of #19716

thanks, removed

girarda

🚢

darynaishchenko · 2022-12-13T10:05:28Z

/test connector=connectors/source-metabase

🕑 connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3684334801
✅ connectors/source-metabase https://github.com/airbytehq/airbyte/actions/runs/3684334801
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     208     92    56%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-280, 288-301, 306-312, 319-330, 337-353
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1599    332    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: This connector does not implement incremental sync
======================== 29 passed, 1 skipped in 44.73s ========================

lazebnyi · 2022-12-13T19:57:11Z

/publish connector=connectors/source-metabase run-tests=false

🕑 Publishing the following connectors:
connectors/source-metabase
https://github.com/airbytehq/airbyte/actions/runs/3688872373

Connector	Did it publish?	Were definitions generated?
connectors/source-metabase	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

migrated to YAMl

3c0d4fa

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Nov 9, 2022

octavia-squidington-iv added the connectors/source/metabase label Nov 9, 2022

darynaishchenko temporarily deployed to more-secrets November 9, 2022 17:10 Inactive

added changelog

2f90309

darynaishchenko linked an issue Nov 9, 2022 that may be closed by this pull request

Source Metabase: Migrate connector from Alpha (Python) to Beta (YAML) #18706

Closed

darynaishchenko requested review from bazarnov, grubberr and davydov-d November 9, 2022 17:12

darynaishchenko temporarily deployed to more-secrets November 9, 2022 17:13 Inactive

davydov-d requested changes Nov 9, 2022

View reviewed changes

davydov-d reviewed Nov 10, 2022

View reviewed changes

added unit tests, refactor source, fixed ecpected records

3a7c3d7

darynaishchenko temporarily deployed to more-secrets November 10, 2022 14:46 Inactive

darynaishchenko requested a review from davydov-d November 10, 2022 14:47

davydov-d reviewed Nov 10, 2022

View reviewed changes

fixed working with session token

dd17096

darynaishchenko requested a review from davydov-d November 18, 2022 09:00

darynaishchenko temporarily deployed to more-secrets November 18, 2022 09:02 Inactive

Merge branch 'master' into daryna/metabase-to-beta-yaml

c3a8bfb

darynaishchenko temporarily deployed to more-secrets November 18, 2022 09:44 Inactive

added creating new session token if user passed invalid

76a1632

darynaishchenko force-pushed the daryna/metabase-to-beta-yaml branch from 30ed0b2 to 76a1632 Compare November 18, 2022 10:40

darynaishchenko temporarily deployed to more-secrets November 18, 2022 10:42 Inactive

lazebnyi requested a review from girarda November 18, 2022 14:06

girarda requested changes Nov 19, 2022

View reviewed changes

davydov-d approved these changes Nov 21, 2022

View reviewed changes

darynaishchenko mentioned this pull request Nov 22, 2022

Low-Code: added SessionTokenAuthenticator #19716

Merged

girarda requested a review from brianjlai November 28, 2022 18:14

darynaishchenko added 2 commits December 7, 2022 16:29

removed custom auth in components, now it's in cdk

a66e9da

Merge branch 'master' into daryna/metabase-to-beta-yaml

7fb84f0

darynaishchenko temporarily deployed to more-secrets December 7, 2022 14:33 — with GitHub Actions Inactive

darynaishchenko temporarily deployed to more-secrets December 7, 2022 14:34 — with GitHub Actions Inactive

darynaishchenko requested a review from girarda December 7, 2022 14:34

girarda reviewed Dec 8, 2022

View reviewed changes

added config for auth, deleted unused tests and dependancies

c23b2ed

darynaishchenko temporarily deployed to more-secrets December 12, 2022 20:18 — with GitHub Actions Inactive

lazebnyi requested a review from girarda December 12, 2022 20:31

girarda approved these changes Dec 12, 2022

View reviewed changes

bazarnov changed the title ~~Source Metabase: migrate to Beta YAMl~~ Source Metabase: migrate to Beta YAML Dec 13, 2022

auto-bump connector version

36d9a78

octavia-squidington-iii temporarily deployed to more-secrets December 13, 2022 20:11 — with GitHub Actions Inactive

Merge branch 'master' into daryna/metabase-to-beta-yaml

c4d90d0

lazebnyi merged commit 469199a into master Dec 13, 2022

lazebnyi deleted the daryna/metabase-to-beta-yaml branch December 13, 2022 20:18

github-actions bot mentioned this pull request Dec 15, 2022

Bump helm chart version reference to 0.43.5 #20526

Closed

octavia-squidington-iii mentioned this pull request Dec 18, 2022

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged



		@dataclass
		class MetabaseAuth(HttpAuthenticator, JsonSchemaMixin):



		@dataclass
		class MetabaseSessionTokenAuthenticator(AbstractHeaderAuthenticator, DeclarativeAuthenticator, JsonSchemaMixin):

Source Metabase: migrate to Beta YAML #19236

Source Metabase: migrate to Beta YAML #19236

Conversation

darynaishchenko commented Nov 9, 2022

darynaishchenko commented Nov 9, 2022 • edited by github-actions bot Loading

Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darynaishchenko commented Nov 18, 2022 • edited by github-actions bot Loading

Build Failed

darynaishchenko commented Nov 18, 2022 • edited by github-actions bot Loading

Build Failed

darynaishchenko commented Nov 18, 2022 • edited by github-actions bot Loading

Build Failed

girarda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda left a comment

Choose a reason for hiding this comment

darynaishchenko commented Dec 13, 2022 • edited by github-actions bot Loading

Build Passed

lazebnyi commented Dec 13, 2022 • edited by github-actions bot Loading

darynaishchenko commented Nov 9, 2022 •

edited by github-actions bot

Loading

darynaishchenko commented Nov 18, 2022 •

edited by github-actions bot

Loading

darynaishchenko commented Nov 18, 2022 •

edited by github-actions bot

Loading

darynaishchenko commented Nov 18, 2022 •

edited by github-actions bot

Loading

darynaishchenko commented Dec 13, 2022 •

edited by github-actions bot

Loading

lazebnyi commented Dec 13, 2022 •

edited by github-actions bot

Loading