connectors-qa: add check for 'maxSecondsBetweenMessages' presence in certified connectors metadata #36803

DanyloGL · 2024-04-03T16:47:16Z

All certified source connectors must have maximum request rate limit reset time set in their metadata.yaml files.

What

Added QA check for presence of 'maxSecondsBetweenMessages' value in 'metadata.yaml' files for all certified source python and low-code connectors.

How

added new check to metadata.py file
added tests to test_metadata.py file
updated documents: qa-checks.md file

🚨 User Impact 🚨

No breaking changes

vercel · 2024-04-03T16:47:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 22, 2024 8:57am

alafanechere · 2024-04-03T16:57:39Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

@@ -149,8 +149,33 @@ def _run(self, connector: Connector) -> CheckResult:
        )


+class CheckConnectorMaxSecondsBetweenMessagesValue(MetadataCheck):
+    name = "Certified connector must have a value filled out for maxSecondsBetweenMessages in metadata"
+    description = f"Certified connectors must have a value filled out for maxSecondsBetweenMessages in metadata. It must be set in the 'data' field in {consts.METADATA_FILE_NAME}"


Could you please add more details about the purpose of this field, why its important etc.?

alafanechere · 2024-04-03T16:58:33Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+class CheckConnectorMaxSecondsBetweenMessagesValue(MetadataCheck):
+    name = "Certified connector must have a value filled out for maxSecondsBetweenMessages in metadata"
+    description = f"Certified connectors must have a value filled out for maxSecondsBetweenMessages in metadata. It must be set in the 'data' field in {consts.METADATA_FILE_NAME}"
+    applies_to_connector_languages = [ConnectorLanguage.PYTHON, ConnectorLanguage.LOW_CODE]


Wouldn't it make sense to remove this line so that it runs for java connectors too?

alafanechere · 2024-04-03T17:01:46Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+    applies_to_connector_languages = [ConnectorLanguage.PYTHON, ConnectorLanguage.LOW_CODE]
+
+    @staticmethod
+    def check_connector_certified(connector: Connector) -> bool:


I think this check should run on all connectors.

All certified should be covered, what is the purpose of having this for the community sources?

We want to enforce best practices on new community PRs too.

I don't have a strong opinion. Say we run this on all community connectors — will their CI be red on all pull requests moving forward unless we add the field? We'd end up just adding the field with some dummy value, because knowing actual value requires tedious work of actually reading the API docs.

I'd say it should be a requirement to be published on Cloud, so perhaps certified is alright?

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

natikgadzhi

Left some comments — I like the idea, but I'll let Augustin to support this through the review.

Either way, please add the changelog + version.

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

natikgadzhi · 2024-04-04T23:12:15Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+    applies_to_connector_languages = [ConnectorLanguage.PYTHON, ConnectorLanguage.LOW_CODE]
+
+    @staticmethod
+    def check_connector_certified(connector: Connector) -> bool:


I don't have a strong opinion. Say we run this on all community connectors — will their CI be red on all pull requests moving forward unless we add the field? We'd end up just adding the field with some dummy value, because knowing actual value requires tedious work of actually reading the API docs.

I'd say it should be a requirement to be published on Cloud, so perhaps certified is alright?

natikgadzhi · 2024-04-04T23:13:15Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+    @staticmethod
+    def check_connector_certified(connector: Connector) -> bool:
+        connector_sl_value = connector.metadata.get("ab_internal", {}).get("sl", 0)
+        return connector_sl_value >= 200


I don't know if certified is the same as connector_sl_value being higher than 200.
I see what you did here — but do you really want that criteria vs perhaps this check should only run on connectors that are published to cloud registry instead? @alafanechere has a method to check for whether connector is in registry in a neighboring PR somewhere.

…sBetweenMessages_value

lazebnyi

lgtm

…sBetweenMessages_value

alafanechere · 2024-04-15T17:28:08Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+    @staticmethod
+    def check_connector_certified(connector: Connector) -> bool:
+        connector_sl_value = connector.metadata.get("ab_internal", {}).get("sl", 0)
+        return connector_sl_value >= 200


You should use the supportLevel instead of the sl_value

Suggested change

return connector_sl_value >= 200

return connector.metadata.get("supportLevel") == "certified"

I think sl_value is not accurate as archived connector still have the sl_value set to 200

alafanechere · 2024-04-15T17:57:31Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py

+    applies_to_connector_types = ["source"]
+
+    @staticmethod
+    def check_connector_certified(connector: Connector) -> bool:


I think this method should move up to the Check abstract class and you should add a runs_on_certified_connectors property and change Check.run method to skip there if the connector is not certified. (same logic as if connector.language not in self.applies_to_connector_languages:)

lazebnyi

lgtm

alafanechere

Thanks for the changes!
Just a minor comment for maintainability.

alafanechere · 2024-04-17T13:19:52Z

airbyte-ci/connectors/connectors_qa/src/connectors_qa/models.py

@@ -21,6 +21,8 @@

 ALL_TYPES = ["source", "destination"]

+ALL_SUPPORT_LEVELS = ["certified", "community"]


nit: if we add a new support level we would have to update this list.
We could make applies_to_connector_support_levels return None by default which would me "applies to all support levels"

…_maxSecondsBetweenMessages_value' into Danylo_GL/QA_check_metadata_file_maxSecondsBetweenMessages_value

…sBetweenMessages_value

…_maxSecondsBetweenMessages_value' into Danylo_GL/QA_check_metadata_file_maxSecondsBetweenMessages_value

…sBetweenMessages_value

…certified connectors metadata (airbytehq#36803) Co-authored-by: Natik Gadzhi <natik@respawn.io>

DanyloGL added 4 commits April 3, 2024 18:21

Update metadata.py

3257006

Update test_metadata.py

349b811

Update qa-checks.md

0bf778f

Update qa-checks.md

0d309e6

DanyloGL requested a review from a team as a code owner April 3, 2024 16:47

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 3, 2024

DanyloGL requested review from lazebnyi and a team April 3, 2024 16:47

DanyloGL added 3 commits April 3, 2024 19:50

Update metadata.py

a08ac7f

Update metadata.py

c2cadf3

Update test_metadata.py

16e9fa6

DanyloGL requested a review from bazarnov April 3, 2024 17:01

DanyloGL self-assigned this Apr 3, 2024

alafanechere requested changes Apr 3, 2024

View reviewed changes

DanyloGL removed their assignment Apr 3, 2024

alafanechere reviewed Apr 4, 2024

View reviewed changes

airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/metadata.py Outdated Show resolved Hide resolved

DanyloGL added 4 commits April 4, 2024 17:31

Update metadata.py

290c903

Update metadata.py

4f2572e

Update metadata.py

a349d26

Update metadata.py

849fc30

natikgadzhi reviewed Apr 4, 2024

View reviewed changes

DanyloGL mentioned this pull request Apr 5, 2024

Set maxSecondsBetweenMessages value for Java source certified connectors #36857

Closed

DanyloGL added 3 commits April 9, 2024 18:07

Update pyproject.toml

1a2f045

Update pyproject.toml

f29dda4

Merge branch 'master' into Danylo_GL/QA_check_metadata_file_maxSecond…

3f235d8

…sBetweenMessages_value

vercel bot deployed to Preview April 9, 2024 15:41 View deployment

DanyloGL added 3 commits April 9, 2024 18:44

Update metadata.py

d3e676b

Update metadata.py

8518d66

Update qa-checks.md

02118fb

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 11, 2024

vercel bot deployed to Preview April 11, 2024 15:07 View deployment

update docs

bab68ae

vercel bot deployed to Preview April 11, 2024 15:27 View deployment

lazebnyi requested a review from natikgadzhi April 11, 2024 21:33

lazebnyi approved these changes Apr 11, 2024

View reviewed changes

Merge branch 'master' into Danylo_GL/QA_check_metadata_file_maxSecond…

6cc0010

…sBetweenMessages_value

vercel bot deployed to Preview April 12, 2024 10:15 View deployment

alafanechere requested changes Apr 15, 2024

View reviewed changes

DanyloGL and others added 3 commits April 16, 2024 16:45

chore: format code

582ad20

chore: format code

067afde

Update qa-checks.md

e458de5

vercel bot deployed to Preview April 16, 2024 17:28 View deployment

DanyloGL requested review from alafanechere and lazebnyi April 17, 2024 12:02

lazebnyi approved these changes Apr 17, 2024

View reviewed changes

alafanechere approved these changes Apr 17, 2024

View reviewed changes

DanyloGL and others added 3 commits April 18, 2024 19:06

chore: format code

62e197e

Merge remote-tracking branch 'origin/Danylo_GL/QA_check_metadata_file…

020ac42

…_maxSecondsBetweenMessages_value' into Danylo_GL/QA_check_metadata_file_maxSecondsBetweenMessages_value

Merge branch 'master' into Danylo_GL/QA_check_metadata_file_maxSecond…

15fbb2e

…sBetweenMessages_value

vercel bot deployed to Preview April 18, 2024 16:21 View deployment

DanyloGL added 2 commits April 18, 2024 19:41

chore: format code

d9f50d3

Merge remote-tracking branch 'origin/Danylo_GL/QA_check_metadata_file…

a743d3e

…_maxSecondsBetweenMessages_value' into Danylo_GL/QA_check_metadata_file_maxSecondsBetweenMessages_value

vercel bot deployed to Preview April 18, 2024 17:37 View deployment

DanyloGL requested a review from alafanechere April 18, 2024 18:43

Merge branch 'master' into Danylo_GL/QA_check_metadata_file_maxSecond…

51187d0

…sBetweenMessages_value

vercel bot deployed to Preview April 22, 2024 08:57 View deployment

lazebnyi merged commit 17a374e into master Apr 25, 2024
35 checks passed

lazebnyi deleted the Danylo_GL/QA_check_metadata_file_maxSecondsBetweenMessages_value branch April 25, 2024 12:17

FVidalCarneiro pushed a commit to AgiData/airbyte that referenced this pull request May 2, 2024

connectors-qa: add check for 'maxSecondsBetweenMessages' presence in …

dccba8a

…certified connectors metadata (airbytehq#36803) Co-authored-by: Natik Gadzhi <natik@respawn.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectors-qa: add check for 'maxSecondsBetweenMessages' presence in certified connectors metadata #36803

connectors-qa: add check for 'maxSecondsBetweenMessages' presence in certified connectors metadata #36803

DanyloGL commented Apr 3, 2024

vercel bot commented Apr 3, 2024 •

edited

Loading

alafanechere Apr 3, 2024

alafanechere Apr 3, 2024

bazarnov Apr 3, 2024

alafanechere Apr 3, 2024

bazarnov Apr 3, 2024 •

edited

Loading

alafanechere Apr 4, 2024

natikgadzhi Apr 4, 2024

natikgadzhi left a comment

natikgadzhi Apr 4, 2024

natikgadzhi Apr 4, 2024

lazebnyi left a comment

alafanechere Apr 15, 2024

alafanechere Apr 15, 2024

lazebnyi left a comment

alafanechere left a comment

alafanechere Apr 17, 2024

	return connector_sl_value >= 200
	return connector.metadata.get("supportLevel") == "certified"

		@@ -21,6 +21,8 @@

		ALL_TYPES = ["source", "destination"]

		ALL_SUPPORT_LEVELS = ["certified", "community"]

connectors-qa: add check for 'maxSecondsBetweenMessages' presence in certified connectors metadata #36803

connectors-qa: add check for 'maxSecondsBetweenMessages' presence in certified connectors metadata #36803

Conversation

DanyloGL commented Apr 3, 2024

What

How

🚨 User Impact 🚨

vercel bot commented Apr 3, 2024 • edited Loading

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

bazarnov Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natikgadzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazebnyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazebnyi left a comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 3, 2024 •

edited

Loading

bazarnov Apr 3, 2024 •

edited

Loading