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.

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

🧹 Sweep authSpecification in connectors repo #27996

Merged
merged 16 commits into from Jul 27, 2023

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Jul 6, 2023

What

Relates to airbytehq/airbyte-protocol#39 and https://github.com/airbytehq/airbyte-platform-internal/pull/7595

This PR sweeps all occurrences of authSpecification from the airbytehq/airbyte repository as part of the project to deprecate that field.

How

Remove references to authSpecification, and update the tests as necessary to test the advanced_auth configuration instead.

Note: The legacy authSpecification still appears in the cloud_registry.json, oss_registry.json, and connector_catalog.json files, but this shouldn't matter since those are only used for testing general behavior that doesn't rely on this field, and will be wiped out the next time those files are refreshed due to a catalog shape change.

Recommended reading order

  1. airbyte-api/src/main/openapi/config.yaml - removes authSpecification from the api
  2. airbyte-integrations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_core.py - updates the test_oauth_flow_parameters CAT test to verify that all paths in the advanced_auth object exist in the connector spec, rather than testing something similar for authSpecification
  3. airbyte-integrations/bases/connector-acceptance-test/unit_tests/test_spec.py - updates the CAT unit tests to account for the above change
  4. The rest falls out of the above

🚨 User Impact 🚨

No breaking changes or user impact

@octavia-squidington-iii octavia-squidington-iii added area/api Related to the api area/connectors Connector related issues area/documentation Improvements or additions to documentation area/platform issues related to the platform labels Jul 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • The connector tests are passing in CI
  • You've updated the connector's metadata.yaml file (new!)
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@lmossman lmossman changed the title 🧹 Sweep authSpecification 🧹 Sweep authSpecification in connectors repo Jul 6, 2023
@lmossman
Copy link
Contributor Author

lmossman commented Jul 6, 2023

Note: keeping this in draft because there are still 2 connectors that need to be migrated off of authSpecification: source-rd-station-marketing and destination-google-sheets. This is planned to be done this week, after which this PR can be merged.

But it is ready for review now!

@lmossman
Copy link
Contributor Author

lmossman commented Jul 6, 2023

/legacy-test connector=bases/connector-acceptance-test

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

Build Passed

Test summary info:

All Passed

Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

lgtm!

:returns list of path_in_connector_config paths
"""
paths = []
for value in schema.values():
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the pythonic way to do this is

return [
        "/" + "/".join(value["path_in_connector_config"])
        for value in schema.values()
    ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thanks for the suggestion

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.

lgtm, but once the other two connectors are updated, just make sure to run the legacy-test command and verify the spec test changes still pass

@lmossman
Copy link
Contributor Author

lmossman commented Jul 26, 2023

/legacy-test connector=bases/connector-acceptance-test

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

Build Passed

Test summary info:

All Passed

@lmossman
Copy link
Contributor Author

lmossman commented Jul 26, 2023

/legacy-test connector=connectors/source-rd-station-marketing

🕑 connectors/source-rd-station-marketing https://github.com/airbytehq/airbyte/actions/runs/5663149958
❌ connectors/source-rd-station-marketing https://github.com/airbytehq/airbyte/actions/runs/5663149958
🐛 https://gradle.com/s/uq2jssmvj5dn2

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/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:617: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:714: This tests currently leads to too much failures. We need to fix the connectors at scale first.
============ 3 failed, 33 passed, 4 skipped, 40 warnings in 32.09s =============

@lmossman
Copy link
Contributor Author

lmossman commented Jul 26, 2023

/legacy-test connector=connectors/destination-google-sheets

🕑 connectors/destination-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5663152885
✅ connectors/destination-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5663152885
Python tests coverage:

Name                                       Stmts   Miss  Cover
--------------------------------------------------------------
destination_google_sheets/buffer.py           28      0   100%
destination_google_sheets/__init__.py          2      0   100%
destination_google_sheets/writer.py           42      1    98%
destination_google_sheets/client.py           25      1    96%
destination_google_sheets/spreadsheet.py      47      2    96%
destination_google_sheets/helpers.py          47      4    91%
destination_google_sheets/destination.py      47      9    81%
--------------------------------------------------------------
TOTAL                                        238     17    93%

Build Passed

Test summary info:

All Passed

@lmossman lmossman marked this pull request as ready for review July 26, 2023 00:32
@octavia-squidington-iii
Copy link
Collaborator

destination-mssql-strict-encrypt test report (commit 235f335240) - ❌

⏲️ Total pipeline duration: 31mn55s

Step Result
Validate airbyte-integrations/connectors/destination-mssql-strict-encrypt/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-mssql-strict-encrypt docker image for platform linux/x86_64
Build airbyte/normalization-mssql:dev
./gradlew :airbyte-integrations:connectors:destination-mssql-strict-encrypt:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-mssql-strict-encrypt test

@octavia-squidington-iii
Copy link
Collaborator

source-facebook-marketing test report (commit 235f335240) - ❌

⏲️ Total pipeline duration: 61mn41s

Step Result
Validate airbyte-integrations/connectors/source-facebook-marketing/metadata.yaml
Connector version semver check
Connector version increment check
QA checks
Code format checks
Connector package install
Build source-facebook-marketing docker image for platform linux/x86_64
Unit tests
Integration tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-facebook-marketing test

@octavia-squidington-iii
Copy link
Collaborator

destination-exasol test report (commit 4a754eace5) - ❌

⏲️ Total pipeline duration: 15mn00s

Step Result
Validate airbyte-integrations/connectors/destination-exasol/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-exasol docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:destination-exasol:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-exasol test

@octavia-squidington-iii
Copy link
Collaborator

destination-pubsub test report (commit 4a754eace5) - ✅

⏲️ Total pipeline duration: 18mn35s

Step Result
Validate airbyte-integrations/connectors/destination-pubsub/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-pubsub docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:destination-pubsub:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-pubsub test

@octavia-squidington-iii
Copy link
Collaborator

destination-databricks test report (commit 4a754eace5) - ❌

⏲️ Total pipeline duration: 55mn16s

Step Result
Validate airbyte-integrations/connectors/destination-databricks/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-databricks docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:destination-databricks:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-databricks test

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5682255151
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5682255151
🐛

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5682751535
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5682751535
🐛

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-publish connector=bases/connector-acceptance-test

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


Connector Version Did it publish?
bases/connector-acceptance-test

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

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5683103879
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5683103879
🐛

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-publish connector=bases/connector-acceptance-test

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


Connector Version Did it publish?
bases/connector-acceptance-test

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

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5683728761
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5683728761
🐛

@lmossman
Copy link
Contributor Author

lmossman commented Jul 27, 2023

/legacy-test connector=connectors/source-google-sheets

🕑 connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5684860213
❌ connectors/source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/5684860213
🐛 https://gradle.com/s/tkiw33piverke

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs2] - docker.errors.Cont...
FAILED test_core.py::TestDiscovery::test_discover[inputs0] - docker.errors.Co...
ERROR test_core.py::TestDiscovery::test_defined_cursors_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_refs_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-allOf]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-not]
ERROR test_core.py::TestDiscovery::test_primary_keys_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_streams_has_sync_modes[inputs0] - doc...
ERROR test_core.py::TestDiscovery::test_additional_properties_is_true[inputs0]
ERROR test_core.py::TestDiscovery::test_backward_compatibility[inputs0] - doc...
ERROR test_core.py::TestBasicRead::test_read[inputs0] - docker.errors.Contain...
ERROR test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: Incremental sync are not supported on this connector
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:105: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:725: This tests currently leads to too much failures. We need to fix the connectors at scale first.
============= 2 failed, 27 passed, 3 skipped, 10 errors in 57.09s ==============

@octavia-squidington-iii
Copy link
Collaborator

source-google-sheets test report (commit bbb38c11a7) - ❌

⏲️ Total pipeline duration: 05mn02s

Step Result
Validate airbyte-integrations/connectors/source-google-sheets/metadata.yaml
Connector version semver check
Connector version increment check
QA checks
Code format checks
Connector package install
Build source-google-sheets docker image for platform linux/x86_64
Unit tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-google-sheets test

@octavia-squidington-iii
Copy link
Collaborator

destination-r2 test report (commit bbb38c11a7) - ❌

⏲️ Total pipeline duration: 08mn06s

Step Result
Validate airbyte-integrations/connectors/destination-r2/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-r2 docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:destination-r2:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-r2 test

@octavia-squidington-iii
Copy link
Collaborator

source-snowflake test report (commit bbb38c11a7) - ❌

⏲️ Total pipeline duration: 10mn27s

Step Result
Validate airbyte-integrations/connectors/source-snowflake/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build source-snowflake docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:source-snowflake:integrationTest
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-snowflake test

@octavia-squidington-iii
Copy link
Collaborator

source-postgres test report (commit bbb38c11a7) - ✅

⏲️ Total pipeline duration: 14mn30s

Step Result
Validate airbyte-integrations/connectors/source-postgres/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build source-postgres docker image for platform linux/x86_64
./gradlew :airbyte-integrations:connectors:source-postgres:integrationTest
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-postgres test

@octavia-squidington-iii
Copy link
Collaborator

source-facebook-marketing test report (commit bbb38c11a7) - ❌

⏲️ Total pipeline duration: 61mn49s

Step Result
Validate airbyte-integrations/connectors/source-facebook-marketing/metadata.yaml
Connector version semver check
Connector version increment check
QA checks
Code format checks
Connector package install
Build source-facebook-marketing docker image for platform linux/x86_64
Unit tests
Integration tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-facebook-marketing test

@lmossman
Copy link
Contributor Author

/approve-and-merge reason="source-google-sheets connector should be working since this is just unpinning it to a version that had passed tests on its last change; the source test account is just in a broken state right now"

@octavia-approvington
Copy link
Contributor

Our work here is done
done

@octavia-approvington octavia-approvington merged commit 2c81224 into master Jul 27, 2023
21 of 101 checks passed
@octavia-approvington octavia-approvington deleted the lmossman/deprecate-auth-specification branch July 27, 2023 22:27
@octavia-squidington-iii
Copy link
Collaborator

destination-mssql test report (commit bbb38c11a7) - ❌

⏲️ Total pipeline duration: 304mn01s

Step Result
Validate airbyte-integrations/connectors/destination-mssql/metadata.yaml
Connector version semver check
QA checks
Build connector tar
Build destination-mssql docker image for platform linux/x86_64
Build airbyte/normalization-mssql:dev
./gradlew :airbyte-integrations:connectors:destination-mssql:integrationTest

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=destination-mssql test

@bazarnov
Copy link
Collaborator

@lmossman Hello! It seems like this change break some more source connectors, like:

  • source-github - test
  • source-instagram - test
  • source-zendesk-support - test
  • source-zendesk-talk - test

They are failed on CAT with this:

AssertionError: Specified oauth fields are missed from spec schema: {'/credentials/client_secret', '/credentials/client_id'}
E       assert equals failed
E         set(^[                       set(^)                      
E         - '/credentials/client_id',                             
E         - '/credentials/client_secr                             
E         -et',                                                   
E         -])

Assuming that client_id and client_secret fields are available in the oneOf / or root connector's spec. However this was not an issue 3 days ago, I wonder why this is happening now, could you please advice?

@bazarnov
Copy link
Collaborator

UPDATED:

Apparently some sources like Trello used to return some server_input_configuration values to the:

"complete_oauth_server_output_specification": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "client_id": {
            "type": "string",
            "path_in_connector_config": ["client_id"]
          },
          "client_secret": {
            "type": "string",
            "path_in_connector_config": ["client_secret"]
          }
        }
      }

Removing this complete_oauth_server_output_specification resolved the CAT issues. because there is nowhere to return the values for the input config, and they are not used elsewhere but for OAuth Flow server input.

I guess the rest of the failed sources CATs died for the same reason, fixing them now.

@lmossman
Copy link
Contributor Author

lmossman commented Jul 31, 2023

@bazarnov that sounds right to me; for example, looking at source-github's spec, it looks like this field is pointing at the path [credentials, client_id], but that path doesn't actually exist in the connectionSpecification

@bazarnov
Copy link
Collaborator

it is, however the Connectors Team was not aware of this change) It would be very helpful if we know that prior to the merge, if it's possible)

@lmossman
Copy link
Contributor Author

Yes I definitely could have communicated this better, sorry about that!

I didn't expect this change to cause new failures in connectors since it should have been testing the same type of things as the old authSpecification test, but in these cases the connectors failed the new test because of an underlying bug in their spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/connectors Connector related issues area/documentation Improvements or additions to documentation area/platform issues related to the platform connectors/source/facebook-marketing connectors/source/google-sheets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants