-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 linkedin ads: Migrate to May 2023 #26372
🚨🚨 Source linkedin ads: Migrate to May 2023 #26372
Conversation
Before Merging a Connector Pull RequestWow! 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:
If the checklist is complete, but the CI check is failing,
|
# Conflicts: # airbyte-config-oss/init-oss/src/main/resources/seed/oss_registry.json
/test connector=connectors/source-linkedin-ads
Build FailedTest summary info:
|
/test connector=connectors/source-linkedin-ads
Build PassedTest summary info:
|
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.
Huge change! Looks good!
@artem1205 could you share the link to the breaking changes checklist for this change? |
description updated |
@@ -6,8 +6,7 @@ | |||
from setuptools import find_packages, setup | |||
|
|||
MAIN_REQUIREMENTS = [ | |||
"airbyte-cdk~=0.1", | |||
"pendulum~=2.1", | |||
"airbyte-cdk", |
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.
could you pin to a minor version?
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.
GA connector should always use latest CDK version, isn't it?
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.
latest minor version. Since major version changes are breaking we shouldn't "automatically" update this connector if we release a major CDK version
@@ -336,3 +338,34 @@ def transform_data(records: List) -> Iterable[Mapping]: | |||
record = transform_col_names(record, DESTINATION_RESERVED_KEYWORDS) | |||
|
|||
yield record | |||
|
|||
|
|||
def _encode_params(data): |
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.
can you clarify in the docstring a few things:
- why this is here instead of using the implementation in
requests
- this is being used to override a private method in
RequestEncodingMixin
-- this is a smell and could break if we upgrade therequests
library. Is there any way around doing it that way?
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.
refactored; will pass already encoded request params from now
pass already encoded request params
/test connector=connectors/source-linkedin-ads
Build PassedTest summary info:
|
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.
In the rollout announcement is there a link we can point users to so they can know exactly which streams were changed/fields were added or removed? ideally it's a link to Linkedin's docs about the change
the only link we have is a migration guide and short video about versioning. |
# Conflicts: # airbyte-integrations/connectors/source-linkedin-ads/Dockerfile # airbyte-integrations/connectors/source-linkedin-ads/metadata.yaml # docs/integrations/sources/linkedin-ads.md
* Source LinkedIn Ads: update CDK version * Source LinkedIn Ads: update schemas * Source LinkedIn Ads: refactor source * Source LinkedIn Ads: Migrate to May 2023 * Source LinkedIn Ads: monkeypatch url encoding to bypass encode :,%() * Source LinkedIn Ads: fix test * Source LinkedIn Ads: fix unittest * Source Pinterest: refactor * Source LinkedIn Ads: update docstrings; Refactor * Source LinkedIn Ads: bump version * Source LinkedIn Ads: update docs * Source LinkedIn Ads: update schemas * Automated Change * Source LinkedIn Ads: update schemas; refactor * Source LinkedIn Ads: update expected records * Source LinkedIn Ads: refactor * Source LinkedIn Ads: fix unit test * Source LinkedIn Ads: fix acceptance test config * Source LinkedIn Ads: update source * Source LinkedIn Ads: update abnormal state * Source LinkedIn Ads: add TypeTransformer + fix schema * Source LinkedIn Ads: Format code * Source LinkedIn Ads: update expected records * Source LinkedIn Ads: update expected records * Source LinkedIn Ads: update doc links * Source LinkedIn Ads: pin CDK version * Source LinkedIn Ads: refactor; pass already encoded request params * Source LinkedIn Ads: fix acceptance test versioning --------- Co-authored-by: artem1205 <artem1205@users.noreply.github.com>
What
Resolve #23609
Migrate to LinkedIn API version: May 2023
How
Migrate to LinkedIn API version: May 2023
Change Release Playbook: https://docs.google.com/document/d/1I8-4emBqKF8JiS3f3yET91MLNZSMZg_yLapaEGU927c/edit#
Recommended reading order
y.python
🚨 User Impact 🚨
Removed Streams:
Schema changed in streams:
State changed in stream:
lastModified
->lastModifiedAt
Pre-merge Actions
Updating a connector
Community member or Airbyter
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.