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.

Sign up for GitHub

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

Jump to bottom

🎉 CAT: Add allowedHosts and suggestedSteams checks for metadata.yaml for connectors ql >= 400 #34358

Merged
merged 9 commits into from
Jan 26, 2024
Merged

🎉 CAT: Add allowedHosts and suggestedSteams checks for metadata.yaml for connectors ql >= 400 #34358

merged 9 commits into from
Jan 26, 2024

Conversation

bazarnov
Copy link
Collaborator

@bazarnov bazarnov commented Jan 18, 2024 •
edited
Loading

What

Resolving: #33566

How

  • added two new tests to connector_attribute test suite:
    • test_certified_connector_has_allowed_hosts
    • test_certified_connector_has_suggested_streams
  • covered cases with corresponding unit_tests

Tests

The tests were running against source-faker, with modified acceptance-test-config.yml:

connector_image: airbyte/source-faker:dev
acceptance_tests:
  spec:
    tests:
      - spec_path: "integration_tests/spec.json"
  connector_attributes:
    tests:
      - config_path: "secrets/config.json"
        allowed_hosts:
           bypass_reason: "Description of why the `AllowedHosts` check is skipped"
        suggested_streams:
           bypass_reason: "Description of why the `SuggestedStreams` check is skipped"
  ...
  ...

To check the use-cases, please refer to the unit_tests.

🚨 User Impact 🚨

No impact is expected, to enable additional metadata.yaml checks, the connector_attributes config, should be supplied in the acceptance-test-config.yml as shown above, for each target connector.

@bazarnov bazarnov self-assigned this Jan 18, 2024
@bazarnov bazarnov requested a review from a team January 18, 2024 18:25
@vercel Vercel
Copy link

vercel bot commented Jan 18, 2024 •
edited
Loading

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 Jan 26, 2024 1:51pm

@bazarnov bazarnov changed the title 🎉 CAT: Add allowedHosts and suggestedSteams checks for metadata.yaml for connectors >= ql: 400 🎉 CAT: Add allowedHosts and suggestedSteams checks for metadata.yaml for connectors ql >= 400 Jan 18, 2024
@bazarnov bazarnov linked an issue Jan 18, 2024 that may be closed by this pull request
Add CAT scenario that validates connector's metadata.yaml specifies allowed hosts and suggested streams #33566
Closed
3 tasks
@bazarnov bazarnov requested review from a team January 18, 2024 18:29
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jan 22, 2024
brianjlai
brianjlai reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@brianjlai brianjlai left a comment

Choose a reason for hiding this comment

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

a couple of nits and suggestions, will approve afterwards

airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/config.py Outdated
@@ -197,6 +209,10 @@ class ConnectorAttributesConfig(BaseConfig):
streams_without_primary_key: Optional[List[NoPrimaryKeyConfiguration]] = Field(
description="Streams that do not support a primary key such as reports streams"
)
allowed_hosts: Optional[AllowedHostsConfiguration] = Field(
description="The list of external whitelisted resources the connector is allowed to communicate with."
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the suggested_streams descriptions aren't quite accurate. These are describing how the field is used in the metadata.yaml, but we should actually use a description that this setting is used when a source does not have external resources for example:

"Used to bypass checking the allowedHosts field in a source's metadata.yaml when all external hosts should be reachable"

And the same for suggested_streams, we should say that this is intended to be the override field not the suggested streams themselves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py Outdated
metadata = connector_metadata.get("data", {})

has_allowed_hosts_property = "allowedHosts" in metadata.keys()
assert has_allowed_hosts_property, f"The `AllowedHosts` property is missing in `metadata.data` for `metadata.yaml`."
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: but in metadata.yaml is defined as camel case not capitalized: allowedHosts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected.

...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py Show resolved Hide resolved
@bazarnov bazarnov requested a review from brianjlai January 25, 2024 10:54
@bazarnov
Copy link
Collaborator Author

@brianjlai The review comments were considered, please checkout.

@bazarnov bazarnov requested a review from maxi297 January 26, 2024 13:33
@bazarnov
Copy link
Collaborator Author

@maxi297 Could you please finalize the review from the part of Airbyte? Thank you.

maxi297
maxi297 approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

:shipit:

@bazarnov bazarnov merged commit 7594c36 into master Jan 26, 2024
25 checks passed
@bazarnov bazarnov deleted the baz/baz/cat/add-allowedhosts-suggestedstreams-tests branch January 26, 2024 13:57
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 21, 2024
@bazarnov @jatinyadav-cc
🎉 CAT: Add allowedHosts and suggestedSteams checks for `metadata.…
844310b 
…yaml` for connectors `ql >= 400` (airbytehq#34358)
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
@bazarnov @jatinyadav-cc
🎉 CAT: Add allowedHosts and suggestedSteams checks for `metadata.…
cb49436 
…yaml` for connectors `ql >= 400` (airbytehq#34358)
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
@bazarnov @jatinyadav-cc
🎉 CAT: Add allowedHosts and suggestedSteams checks for `metadata.…
0e47d00 
…yaml` for connectors `ql >= 400` (airbytehq#34358)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darynaishchenko darynaishchenko darynaishchenko approved these changes

@artem1205 artem1205 artem1205 approved these changes

@maxi297 maxi297 maxi297 approved these changes

@lazebnyi lazebnyi Awaiting requested review from lazebnyi

@oustynova oustynova Awaiting requested review from oustynova

@brianjlai brianjlai Awaiting requested review from brianjlai

Assignees

@bazarnov bazarnov

Labels
area/documentation Improvements or additions to documentation team/connectors-python
Projects
No open projects
GL Roadmap
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CAT scenario that validates connector's metadata.yaml specifies allowed hosts and suggested streams
6 participants
@bazarnov @maxi297 @artem1205 @brianjlai @darynaishchenko @octavia-squidington-iii