-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🎉 New Source: LinkedIn Leads #42101
🎉 New Source: LinkedIn Leads #42101
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Original discussion: #42042 |
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 fix the title of your pull request to Source instead of Destination?
Done, sorry for that :) |
Just fix formatting via |
type: DpathExtractor | ||
field_path: | ||
- elements | ||
partition_router: |
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.
Is that the same account_id as in linkedin_ads?
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.
@natikgadzhi yes, why ?
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.
@FVidalCarneiro this is excellent work, and don't get me wrong — I am EXTREMELY grateful to have this.
I want this merged, alongside your other PRs and ideas. Here's how we can do this:
- Make this a new stream in source-linkedin-ads
- I left some comments re: possibility of incremental behavior, but it's okay to start with full refresh
- schemas should be inline.
type: object | ||
$schema: http://json-schema.org/draft-07/schema# | ||
required: | ||
- client_id |
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.
If leads require new scopes compared to linkedin_ads, this would need to be a breaking change cc @FVidalCarneiro
lead_form_response: true | ||
|
||
schemas: | ||
lead_form_response: |
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.
This looks incomplete — I would expect schemas to be inline in the yaml manifest, if the connector is made in Builder.
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.
@@ -0,0 +1,284 @@ | |||
{ |
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.
Don't do separate json schemas, let's put them in the manifest.
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.
@natikgadzhi why do you prefer this way of working with inline schemas ? IMHO, it is more readable to split them in distinct files and not overload the manifest 🤔
} | ||
} | ||
}, | ||
"created": { |
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.
It has created field that looks a lot like a timestamp. Can we use this for incremental sync?
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.
@natikgadzhi we can not, according to the official documentation we can only use submittedAt
filtering, and the issue is, as explained, on the way to build the API call submittedAtTimeRange=(start:1711407600000,end:1711494000000)
which does not look feasible with low code
https://learn.microsoft.com/en-us/linkedin/marketing/lead-sync/leadsync?view=li-lms-2024-06&tabs=http#get-multiple-lead-form-responses
if you have any workaround suggestion 🙏
hello @natikgadzhi @marcosmarxm, please coud you reopen the PR ? |
What
This Pull Request aims in proposing a first version of the
LinkedIn Leads
source connector based on official documentation.How
This PR adds 2 base streams:
This connector was developed using the low-code development framework.
Schemas of the ingested objects are defined in the
schemas/
directory.Review guide
Limits & considerations
section)User Impact
User will have access to 2 streams of a new connector.
As explained in the
Limits & considerations
section of the Documentation and due to low-code capabilities limitations:path
Incremental sync
is not supported because of specific query param expected by LinkedIn API (eg:submittedAtTimeRange=(start:1711407600000,end:1711494000000)
)Also, LinkedIn Ads accounts must be hard-coded in the source connector configuration contrary to the dynamic behavior of LinkedIn Ads source connector. We do consider this spec as acceptable as a first step.
Can this PR be safely reverted and rolled back?