✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304
Conversation
…anges to solve build issues, and changes to solve unit testing issues.
…e config. Updated version to reflect Airbyte versioning standard (this use case is a major version upgrade due to configuration of credentials needed to be changed)
…ebook-marketing-selectable-auth
…ebook-marketing-selectable-auth' into JohnMikkelsonOvative/feature-facebook-marketing-selectable-auth
…eting-selectable-auth
…eting-selectable-auth
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
cristina.mariscal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
octavia-squidington-iii
added
area/connectors
cmm-airbyte
changed the title
|
@cmm-airbyte We need to update the config in the GSM for the change to work, I believe. |
octavia-squidington-iii
added
the
area/documentation
I haven't check the change but it shouldn't be if the change is non-breaking with the config migration |
| @@ -44,6 +44,9 @@ data: | |||
| - "ads_insights_region" | |||
| - "ads_insights_dma" | |||
| - "ads_insights_action_product_id" | |||
| 3.0.0: | |||
There was a problem hiding this comment.
What makes it breaking if we have the config migration? Does that mean that the user will have to opt-in on that change?
There was a problem hiding this comment.
no, I was just guessing this because the PR cannot be directly reverted. I can do only a 2.2.0 version increment. WDYT?
There was a problem hiding this comment.
I think 2.2.0 makes sense, yes! We should still document that the config format changes but there is a migration for backward compatibility but none for forward compatiblity
There was a problem hiding this comment.
great. Thanks Max!
| """ | ||
| credentials = config.get("credentials", None) | ||
| return credentials is None or ( | ||
| "access_token" not in credentials and not ("client_id" in credentials and "client_id" in credentials) |
There was a problem hiding this comment.
This condition feels hard to maintain. If something adds a new auth method, they will need to know that they have to update an old migration script
There was a problem hiding this comment.
mmm.. I think if there is a new change, an iteration over this migration will be required. isn't it?
There was a problem hiding this comment.
Let's take the following scenario: on the 2025-01-04, a developer is doing the work to add username/password as an authentication mechanism. To do so, they update the spec to have credentials.username and credentials.password and configure the authenticator properly in the source.py. Now, this condition will always be triggered which means that the GSM secrets and the database entries will be updated on every operation of the connector. For the source in practice, I don't think it changes that much but I fear that those updates on the platform or GSM side could create issues in the long run. Hence, I would try to make the condition specific to this update. Is there a case where only credentials not being present wouldn't be enough to trigger this migration?
Note if we keep the condition as-is: the "client_id" in credentials and "client_id" in credentials part of the condition should probably be "client_id" in credentials and "client_secret" in credentials
| Starting from `3.0.0`, the `client_id`, `client_secret` and `access_token` will be placed at `credentials` path. | ||
| """ | ||
|
|
||
| message_repository: MessageRepository = InMemoryMessageRepository() |
There was a problem hiding this comment.
This field is useless right now. We can just do print(create_connector_config_control_message(migrated_config).json(exclude_unset=True))
| message_repository: MessageRepository = InMemoryMessageRepository() | ||
|
|
||
| @classmethod | ||
| def should_migrate(cls, config: Mapping[str, Any]) -> bool: |
There was a problem hiding this comment.
It feels like this should be private as it is only used internally. What do you think? The same comment applies to transform, modify_and_save and emit_control_message
There was a problem hiding this comment.
LGTM! Before I approve, I think we had a list of manual tests to do by fixing the version in prod. I don't remember where this list is but can you explicit the tests you have done in the PR description so that we know if there is an issue what we know worked when we released? Once this is shared, I'll approve this change
cmm-airbyte
force-pushed
the
cmm-airbyte/fb_oauth
branch
from
3a6f33b
to
543d5ea
Compare
What
The Source Facebook Marketing Connector does not allow for choosing between providing an access token or using oauth, it only allows for users to define a source connection by following an oauth flow. This does not allow for use-cases in which a large organization may provision an access token to be used to authenticate to the Facebook Marketing data, rather than giving a user's personal Facebook account access.
How
The
specdefines acredentialsfield that is a union between newly createdOAuthCredentialsandServiceAccountCredentials. This allows for the user to select between using the Oauth flow and inserting Service Account Credentials.Review guide
User Impact
The user is able to select between using the Oauth flow and inserting Service Account Credentials when setting up the Facebook Marketing Source.
Can this PR be safely reverted and rolled back?
Source PR here: #37625