-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
CAT: Add scenario that OAuth is default method and fields are marked as airbyte_secret #34178
CAT: Add scenario that OAuth is default method and fields are marked as airbyte_secret #34178
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
Just a couple of questions
...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py
Outdated
Show resolved
Hide resolved
...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/bases/connector-acceptance-test/unit_tests/test_spec.py
Outdated
Show resolved
Hide resolved
…ds-marked-as-secret
@maxi297 updated testing flow
|
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.
Just one clarification and I'm good to go
...te-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py
Show resolved
Hide resolved
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.
two thoughts here:
Do we need a separate CAT acceptance config at all? Rather than force each connector to explicitly define this field, could we just always run the test no matter what connector? And we either skip or pass the test if a connector's spec doesn't have any form of OAuth to validate.
And for the connectors that do have OAuth in the spec, then we verify it's the first option. That seems like the least impact on our existing connectors. And if we're concerned about this test failing for uncertified connectors, we can use operational_certification_test
fixture to skip the test when it quality level is not at the right value
I'm curious if we have seen any use cases for why OAuth shouldn't be first option and it requires a bypass reason?
@brianjlai , For current implementation a separate CAT acceptance config is optional, the test always runs as part of TestSpec scenario. The test is skipped in case 1. no oauth in connector. 2. no oneOf (only one way to auth), if oauth is present 3. auth_default_method.oauth == False with bypass reason provided in CAT acceptance config (A connector health engineer(secondary oncall) will be responsible to fix oauth default method or add skip and bypass in cat config.)
It's no problem to add operational_certification_test fixture, but the reason why it wasn't added was mentioned in the task #33564 (Although this is a higher operational certification level, this is a relatively trivial fix even if it were to block community level connectors.) For example zendesk-talk is certified and generally_available but OAuth is not default option, I didn't find the reason in certification checklist, it needs more investigation that a secondary oncall can do while his work on connector health and provide a fix/bypass.
|
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.
One naming correction on the config validator and there's the merge conflict on the CHANGELOG. Everything else looks good to me.
I think we're in alignment that it will just incorporated w/o the QL taken into account and primary/secondary can fix them if the do come up
airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/config.py
Outdated
Show resolved
Hide resolved
@maxi297, I have approve from @brianjlai. Is everything good from your side? |
…as airbyte_secret (airbytehq#34178)
…as airbyte_secret (airbytehq#34178)
…as airbyte_secret (airbytehq#34178)
What
resolved: #33564
As part of certification criteria we should be ensuring that OAuth on cloud is the first option presented to users and that sensitive values are designated as secrets.
How
OAuth is default: Extended existing TestSpec.test_oauth_flow_parameters() with validation for case when credentials object do have an oneOf list and the first method listed is OAuth method.
OAuth fields marked as secret: TestSpec.test_secret_is_properly_marked has been already implemented. Updated unit test for it with OAuth method in test cases spec.
This has been tested on a handful of different sources:
source-facebook-pages
source-google-ads
source-facebook-marketing
source-github