Extend SATs to capture UI limitations #21451

flash1293 · 2023-01-16T08:28:13Z

Fixes #20063

Changes to the SATs:

Only strings, integers and numbers can be secrets, arrays and objects do not work correctly in the UI. There was an existing related test already which would also accept objects and arrays - made it stricter to not accept them anymore
"type" can't be an array, except for the case where it's two values and the second one is "null" - all other cases are not handled by the UI
The array "items" type can't be an array
The array "items" type either needs to be unspecified which defaults to string, explicitly string, object, number, integer or has an enum defined (other cases won't work in UI)
not, anyOf, itemPrefixes, patternProperties, allOf, if, then, else, dependentSchemas and dependentRequired is not allowed as it won't be rendered as expected
If format: date/date-time is specified, it should have the corresponding pattern set as well and the other way around - this is only a warning as it's not a hard requirement and the UI still works (also enforcing this would require a lot of breaking changes)
An object or array type always needs a valid definition of the sub types (items in case of array, properties in case of object)

There were a few connectors with problematic specs.
For problems that were present before this PR I opened an issue here: https://github.com/airbytehq/airbyte-internal-issues/issues/1291
For new problems I set up a PR here: #21587

github-actions · 2023-01-16T08:30:09Z

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

Run integration tests
Bump connector or module version
Add changelog
Publish the new version

❌ Sources (63)

Connector	Version	Changelog	Publish
`source-airtable`	`1.0.1`	✅	✅
`source-amazon-ads`	`0.1.27`	✅	✅
`source-amazon-seller-partner`	`0.2.30`	✅	✅
`source-amazon-sqs`	`0.1.0`	✅	✅
`source-amplitude`	`0.1.19`	✅	✅
`source-appsflyer`	`0.1.0`	✅	✅
`source-asana`	`0.1.5`	✅	✅
`source-azure-table`	`0.1.3`	✅	✅
`source-braintree`	`0.1.3`	✅	✅
`source-cart`	`0.2.0`	✅	✅
`source-chargebee`	`0.1.16`	✅	✅
`source-commercetools`	`0.1.0`	✅	✅
`source-confluence`	`0.1.1`	✅	✅
`source-datadog`	`0.1.0`	✅	✅
`source-delighted`	`0.2.0`	✅	✅
`source-drift`	`0.2.5`	✅	✅
`source-facebook-marketing`	`0.2.83`	✅	✅
`source-freshcaller`	`0.1.0`	✅	✅
`source-freshsales`	`0.1.2`	✅	✅
`source-freshservice`	`0.1.1`	✅	✅
`source-github`	`0.3.12`	✅	❌ (diff seed version)
`source-gitlab`	`1.0.1`	✅	✅
`source-google-ads`	`0.2.9`	✅	✅
`source-google-search-console`	`0.1.18`	✅	✅
`source-greenhouse`	`0.3.0`	✅	✅
`source-harvest`	`0.1.14`	✅	✅
`source-instagram`	`1.0.1`	✅	✅
`source-iterable`	`0.1.22`	✅	✅
`source-klaviyo`	`0.1.10`	✅	✅
`source-lemlist`	`0.1.1`	✅	✅
`source-lever-hiring`	`0.1.3`	✅	✅
`source-linnworks`	`0.1.5`	✅	✅
`source-mailchimp`	`0.3.1`	✅	✅
`source-mailgun`	`0.1.0`	✅	✅
`source-monday`	`0.2.2`	✅	✅
`source-notion`	`1.0.0`	✅	✅
`source-okta`	`0.1.14`	✅	✅
`source-onesignal`	`0.1.2`	✅	✅
`source-openweather`	`0.1.6`	✅	✅
`source-outreach`	`0.1.2`	✅	✅
`source-pardot`	`0.1.1`	✅	✅
`source-paystack`	`0.1.1`	✅	✅
`source-pinterest`	`0.2.1`	✅	✅
`source-pipedrive`	`0.1.13`	✅	✅
`source-plaid`	`0.3.2`	✅	✅
`source-posthog`	`0.1.8`	✅	✅
`source-prestashop`	`0.3.0`	✅	✅
`source-quickbooks-singer`	`0.1.5`	✅	✅
`source-recharge`	`0.2.4`	✅	✅
`source-retently`	`0.1.3`	✅	✅
`source-salesforce`	`1.0.29`	✅	✅
`source-salesloft`	`0.1.3`	✅	✅
`source-sendgrid`	`0.2.16`	✅	✅
`source-sentry`	`0.1.8`	✅	✅
`source-strava`	`0.1.2`	✅	✅
`source-surveymonkey`	`0.1.13`	✅	✅
`source-tplcentral`	`0.1.1`	✅	✅
`source-twilio`	`0.1.14`	✅	✅
`source-weatherstack`	`0.1.0`	✅	✅
`source-youtube-analytics`	`0.1.3`	✅	✅
`source-zendesk-sunshine`	`0.1.1`	✅	✅
`source-zendesk-talk`	`0.1.5`	✅	✅
`source-zenloop`	`0.1.4`	✅	✅

See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (0)

Connector	Version	Changelog	Publish

See "Actionable Items" below for how to resolve warnings and errors.

✅ Other Modules (0)

Actionable Items

(click to expand)

Category	Status	Actionable Item
Version	❌ mismatch	The version of the connector is different from its normal variant. Please bump the version of the connector.
	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
Changelog	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
	❌ changelog missing	There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish	⚠ not in seed	The connector is not in the seed file (e.g. `source_definitions.yaml`), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.
	❌ diff seed version	The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the `/publish` command to publish the latest version.

…sion

girarda

@alafanechere is there any documentation that should be updated when adding new SATs?

flash1293 · 2023-01-19T17:35:42Z

@girarda Good point, I will set up a separate PR for that!

girarda

lgtm. Added a couple of non-blocking questions and requested a review from Augustin in case he has concerns about the new tests

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

girarda · 2023-01-19T18:36:59Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

+                property_definition = self._get_parent(specification, format_path)
+                pattern = property_definition.get("pattern")
+                if format == "date" and not pattern == DATE_PATTERN:
+                    detailed_logger.warning(


is the plan to migrate existing connectors to always use these patterns and then change this warning to a failure?

It would be nice for consistency, but it felt like too much at the moment. I will create an issue for this.

girarda · 2023-01-19T18:39:15Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

+                format = property_definition.get("format")
+                if not format == "date" and pattern == DATE_PATTERN:
+                    detailed_logger.warning(
+                        f"{pattern_path} is defining a pattern that looks like a date without setting the format to `date`. Consider specifying the format to make it easier for users to edit this field in the UI."


should this fail? What would be a valid format that would trigger a warning?

I was on the fence about this, too - I decided this way because of the following reasoning:

Showing a date picker for a field instead of requiring the user to type it out is more of a presentational/convenience thing than a "just plain wrong/not configurable at all in the UI" thing (like the other checks)

Semantically, a pattern doesn't necessarily mean it's a date - it is very likely for this specific pattern (hence the warning), but is it the right call the enforce this relationship?

One example could be a sort of serial number which has the same format, but is semantically not a date - you wouldn't want to force a connector developer to render a date picker for that.

This is definitely not a strong reasoning, I will put it in the issue I mentioned above about the check the other way.

+1 to @flash1293's reasoning - I had similar reasoning here re: we can't enforce date-formatting on things that just look like they could be dates - its up to the source to define whether we should interpret it as a date or not

girarda

synced offline: arrays of integers are also valid

…sion

flash1293 · 2023-01-20T09:34:21Z

Thanks for the review @girarda ! I updated the checks here and made integers/number valid for arrays

alafanechere

Thanks for these great additions!
I made minor comments.
Requesting change for the single value array topic and type variable name which is a reserved python keyword.
Could you please also share in #dev-connectors-gl when you merge this? This will likely break existing connectors, so if our maintainers can be aware of this addition it will be helpful to debug faster.

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

flash1293 · 2023-01-20T16:02:22Z

Thanks for your comments @alafanechere , I will address them!

This will likely break existing connectors, so if our maintainers can be aware of this addition it will be helpful to debug faster

Hopefully not, I ran these new tests for all sources locally and the only problems that showed up are fixed here: #21587

alafanechere · 2023-01-20T16:04:39Z

Hopefully not, I ran these new tests for all sources locally and the only problems that showed up are fixed here: #21587

Ok then I can approve. Please address the type variable naming and the other comments if you like.

…sion

…e into flash1293/sat-extension

girarda · 2023-01-23T17:43:05Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

@@ -268,7 +269,7 @@ def test_secret_is_properly_marked(self, connector_spec_dict: dict, detailed_log

        if non_secrets_hidden:
            properties = "\n".join(non_secrets_hidden)
-            detailed_logger.warning(
+            pytest.fail(


girarda · 2023-01-23T17:45:26Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py

+        if len(errors) > 0:
+            pytest.fail("\n".join(errors))
+
+    def test_property_type_is_not_array(self, connector_specification: dict):


nit: the type should be Dict

@girarda @flash1293

This test fails with :

def test_property_type_is_not_array(self, connector_specification: dict): E fixture 'connector_specification' not found > available fixtures: acceptance_test_config, actual_connector_spec, base_path, cache, cache_discovered_catalog, cached_schemas, capfd, capfdbinary, caplog, capsys, capsysbinary, class_mocker, configured_catalog, configured_catalog_path, connection_specification, connector_config, connector_config_path, connector_setup, connector_spec, connector_spec_dict, connector_spec_path, cov, detailed_logger, discovered_catalog, docker_runner, doctest_namespace, empty_streams, expect_records_config, expected_records_by_stream, image_tag, invalid_connector_config, invalid_connector_config_path, malformed_connector_config, mocker, module_mocker, monkeypatch, no_cover, package_mocker, previous_cached_schemas, previous_connector_docker_runner, previous_connector_image_name, previous_connector_spec, previous_discovered_catalog, pull_docker_image, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, requests_mock, secret_property_names, session_mocker, skip_backward_compatibility_tests, test_strictness_level, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory > use 'pytest --fixtures [testpath]' for help on them.

…sion

…e into flash1293/sat-extension

…sion

flash1293 · 2023-01-25T18:58:15Z

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/4009061745
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/4009061745
No Python unittests run

Build Passed

Test summary info:

All Passed

flash1293 · 2023-01-25T19:08:08Z

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/4009141222

Connector	Did it publish?	Were definitions generated?
bases/source-acceptance-test	✅	❌

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

This reverts commit fa2c03a.

* Revert "Extend SATs to capture UI limitations (#21451)" This reverts commit fa2c03a. * bump to 0.4.0

…)" This reverts commit 74b5dbf.

* Revert "Revert "Extend SATs to capture UI limitations (#21451)" (#21896)" This reverts commit 74b5dbf. * fix fixture problem

WIP SATs

e9d3c5b

Joe Reuter added 6 commits January 18, 2023 10:15

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

2d33d3b

…sion

adding some more tests

6f3c3ff

improve unit tests

30ef769

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

fd69337

…sion

adjustments

2bec7ba

formatting

ddfc1bf

flash1293 changed the title ~~WIP SATs~~ Extend SATs to capture UI limitations Jan 19, 2023

flash1293 mentioned this pull request Jan 19, 2023

Small spec fixes to make sure they work with connector form UI #21587

Merged

flash1293 added the area/connectors Connector related issues label Jan 19, 2023

flash1293 marked this pull request as ready for review January 19, 2023 11:54

flash1293 requested a review from girarda January 19, 2023 13:36

girarda reviewed Jan 19, 2023

View reviewed changes

girarda requested a review from alafanechere January 19, 2023 18:54

girarda approved these changes Jan 19, 2023

View reviewed changes

girarda requested changes Jan 19, 2023

View reviewed changes

Joe Reuter added 2 commits January 20, 2023 10:17

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

4d19fc0

…sion

allow number and integer for arrays

2800bf7

octavia-squidington-iv removed the area/connectors Connector related issues label Jan 20, 2023

review comments

c1a6b17

alafanechere suggested changes Jan 20, 2023

View reviewed changes

alafanechere approved these changes Jan 20, 2023

View reviewed changes

Joe Reuter added 3 commits January 23, 2023 17:23

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

95d51de

…sion

review comments

55a67bd

Update test_core.py

7b2fd46

flash1293 requested a review from girarda January 23, 2023 17:17

Joe Reuter added 4 commits January 23, 2023 18:26

format

7b8df03

Merge branch 'master' into flash1293/sat-extension

e2f6f6e

format 2

f09fc2b

Merge branch 'flash1293/sat-extension' of github.com:airbytehq/airbyt…

d4d9bdc

…e into flash1293/sat-extension

girarda approved these changes Jan 23, 2023

View reviewed changes

Joe Reuter added 6 commits January 23, 2023 19:39

Merge branch 'master' into flash1293/sat-extension

a6c2d7c

Merge branch 'master' into flash1293/sat-extension

d58d6d9

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

ac93bbc

…sion

Merge branch 'flash1293/sat-extension' of github.com:airbytehq/airbyt…

4b7691e

…e into flash1293/sat-extension

Merge remote-tracking branch 'origin/master' into flash1293/sat-exten…

b41e311

…sion

bump SAT version

5b402aa

flash1293 merged commit fa2c03a into master Jan 25, 2023

flash1293 deleted the flash1293/sat-extension branch January 25, 2023 20:26

github-actions bot mentioned this pull request Jan 25, 2023

Bump helm chart version reference to 0.43.24 #21887

Closed

girarda added a commit that referenced this pull request Jan 26, 2023

Revert "Extend SATs to capture UI limitations (#21451)"

67e3fc0

This reverts commit fa2c03a.

girarda mentioned this pull request Jan 26, 2023

Revert "Extend SATs to capture UI limitations (#21451)" #21896

Merged

37 tasks

girarda added a commit that referenced this pull request Jan 26, 2023

Revert "Extend SATs to capture UI limitations (#21451)" (#21896)

74b5dbf

* Revert "Extend SATs to capture UI limitations (#21451)" This reverts commit fa2c03a. * bump to 0.4.0

flash1293 pushed a commit that referenced this pull request Jan 26, 2023

Revert "Revert "Extend SATs to capture UI limitations (#21451)" (#21896…

6e3809c

…)" This reverts commit 74b5dbf.

flash1293 mentioned this pull request Jan 26, 2023

Extend SATs to capture UI limitations - fixed version #21903

Merged

This was referenced Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #21954

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #21991

Closed

Bump Airbyte version from 0.40.29 to 0.40.30 #22045

Merged

flash1293 pushed a commit that referenced this pull request Jan 30, 2023

Extend SATs to capture UI limitations - fixed version (#21903)

63064d9

* Revert "Revert "Extend SATs to capture UI limitations (#21451)" (#21896)" This reverts commit 74b5dbf. * fix fixture problem

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend SATs to capture UI limitations #21451

Extend SATs to capture UI limitations #21451

flash1293 commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading

❌ Sources (63)

✅ Destinations (0)

✅ Other Modules (0)

Actionable Items

girarda left a comment

flash1293 commented Jan 19, 2023

girarda left a comment

girarda Jan 19, 2023

flash1293 Jan 20, 2023

girarda Jan 19, 2023

flash1293 Jan 20, 2023

erohmensing Feb 1, 2023

girarda left a comment

flash1293 commented Jan 20, 2023

alafanechere left a comment •

edited

Loading

flash1293 commented Jan 20, 2023

alafanechere commented Jan 20, 2023 •

edited

Loading

girarda Jan 23, 2023

girarda Jan 23, 2023

bazarnov Jan 25, 2023

flash1293 commented Jan 25, 2023 •

edited by github-actions bot

Loading

flash1293 commented Jan 25, 2023 •

edited by github-actions bot

Loading

Extend SATs to capture UI limitations #21451

Extend SATs to capture UI limitations #21451

Conversation

flash1293 commented Jan 16, 2023 • edited Loading

github-actions bot commented Jan 16, 2023 • edited Loading

Affected Connector Report

❌ Sources (63)

✅ Destinations (0)

✅ Other Modules (0)

Actionable Items

girarda left a comment

Choose a reason for hiding this comment

flash1293 commented Jan 19, 2023

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

girarda left a comment

Choose a reason for hiding this comment

flash1293 commented Jan 20, 2023

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Jan 20, 2023

alafanechere commented Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Jan 25, 2023 • edited by github-actions bot Loading

Build Passed

flash1293 commented Jan 25, 2023 • edited by github-actions bot Loading

flash1293 commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading

alafanechere left a comment •

edited

Loading

alafanechere commented Jan 20, 2023 •

edited

Loading

flash1293 commented Jan 25, 2023 •

edited by github-actions bot

Loading

flash1293 commented Jan 25, 2023 •

edited by github-actions bot

Loading