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

🐛 Source Snapchat Marketing: remove deprecated auth specification #26358

Conversation

davydov-d
Copy link
Collaborator

@davydov-d davydov-d commented May 22, 2023

What

Remove deprecated authSpecification from connector spec in favour of advancedAuth
#26246

How

Replace authSpecification with advancedAuth

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 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
  • You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
  • 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.

@davydov-d
Copy link
Collaborator Author

davydov-d commented May 22, 2023

/test connector=connectors/source-square

🕑 connectors/source-square https://github.com/airbytehq/airbyte/actions/runs/5048914863
✅ connectors/source-square https://github.com/airbytehq/airbyte/actions/runs/5048914863
Python tests coverage:

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_square/source.py           4      0   100%
source_square/__init__.py         2      0   100%
source_square/components.py      51      4    92%
-------------------------------------------------
TOTAL                            57      4    93%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
================== 48 passed, 2 skipped in 1477.16s (0:24:37) ==================

@davydov-d
Copy link
Collaborator Author

davydov-d commented May 22, 2023

/test connector=connectors/source-snapchat-marketing

🕑 connectors/source-snapchat-marketing https://github.com/airbytehq/airbyte/actions/runs/5048916596
✅ connectors/source-snapchat-marketing https://github.com/airbytehq/airbyte/actions/runs/5048916596
Python tests coverage:

Name                                    Stmts   Miss  Cover
-----------------------------------------------------------
source_snapchat_marketing/__init__.py       2      0   100%
source_snapchat_marketing/source.py       284     22    92%
-----------------------------------------------------------
TOTAL                                     286     22    92%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
================== 44 passed, 1 skipped in 424.22s (0:07:04) ===================

@davydov-d davydov-d marked this pull request as ready for review May 22, 2023 17:59
Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

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

Nice!

@lazebnyi lazebnyi requested review from maxi297 and removed request for alafanechere May 23, 2023 00:10
davydov-d and others added 6 commits May 23, 2023 13:56
…-square-remove-deprecated-auth-specification' of github.com:airbytehq/airbyte into ddavydov/#26246-#26247-source-snapchat-marketing-source-square-remove-deprecated-auth-specification
…-square-remove-deprecated-auth-specification' of github.com:airbytehq/airbyte into ddavydov/#26246-#26247-source-snapchat-marketing-source-square-remove-deprecated-auth-specification
…keting-source-square-remove-deprecated-auth-specification
@davydov-d davydov-d changed the title 🐛 Source Snapchat Marketing, Source Square: remove deprecated auth specification 🐛 Source Snapchat Marketing: remove deprecated auth specification May 24, 2023
@davydov-d
Copy link
Collaborator Author

Removed Source Square from this PR to unblock it as Source Snapchat Marketing is blocked by invalid UI creds

"rootObject": [],
"oauthFlowInitParameters": [["client_id"], ["client_secret"]],
"oauthFlowOutputParameters": [["refresh_token"]]
"advanced_auth": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the same fields are defined by this spec but differently. Is this a breaking change for the user? If so, do we have a plan for migrating the value of these field? This connector is GA so we should make sure the impact is minimal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maxi297 no, this is not a breaking change. Nothing changes for the user. It's just another way to describe oauth flow for the server. Anyway, the task is on hold for now because we're currently not able to log in via Snapchat UI :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as soon as we have access to our account - I'll be able to confirm everything works as expected via manual testing on localhost

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.

Conditional approval given that we have ensured that refresh_token, client_id and client_secret are passed to the new auth spec (see this)

@davydov-d davydov-d closed this Jun 13, 2023
@davydov-d davydov-d reopened this Jun 13, 2023
@alafanechere alafanechere reopened this Jun 13, 2023
@davydov-d davydov-d merged commit 6581b69 into master Jun 13, 2023
28 of 29 checks passed
@davydov-d davydov-d deleted the ddavydov/#26246-#26247-source-snapchat-marketing-source-square-remove-deprecated-auth-specification branch June 13, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants