-
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
feat: Upgrading linkedin ads api version for creatives stream #49458
base: master
Are you sure you want to change the base?
feat: Upgrading linkedin ads api version for creatives stream #49458
Conversation
@FVidalCarneiro is attempting to deploy a commit to the Airbyte Growth Team on Vercel. A member of the Team first needs to authorize 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.
@FVidalCarneiro thanks for update the connector. Update the connector version in the metadata.yaml
and pyproject.toml
files, also add an entry in the connector documentation for this change.
Would be possible to provide access to maintainers edit your branch? This is necessary to run tests, future format fixes and merging the contribution.
Time for another rebase? |
…ce-linkedin-ads-creatives-upgrade-feat # Conflicts: # docs/integrations/sources/linkedin-ads.md
hello @natikgadzhi @lazebnyi, all done ✅ FYI @FVidalCarneiro |
This is quite frustrating not to have a validation for a such small PR |
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.
Code changes are fine, but let's make this a progressive rollout just in case.
@marcosmarxm can you please work with @agarctfi to change the version tags here, and then merge and start a progressive rollout?
I'm happy to force-merge / approve when the version are set to RC correctly.
@natikgadzhi do you confirm we can not help you on this point ? |
hello, new conflict (again), how can we efficiently progress on this topic ? 🙏 |
Hello @natikgadzhi , @marcosmarxm , @agarctfi , another merge conflict that we had to solve. Please try to merge this asap to avoid once again updating our branch. This is a very small change that has a business impact for us and that is taking the creatives endpoint to the direction required (as described in the PR description). We opened this small PR more than a month ago and are a bit frustrated to see it not advancing. FYI, we believe the AdDirectSponsoredContents API has been silently already sunset by Linkedin. On our side, we are currently unable to collect creative names. |
Hello Everyone, Thank you so much, @FVidalCarneiro and @kev-datams, for the adjustment! I was working on this earlier today and just came across your PR—great timing. To the Airbyte team, is there any update on when this might be merged? It’s a valuable feature for my team, and I’m exploring options in the meantime. Appreciate your efforts—thanks in advance! ps: let me know if I can be of any help |
@agarctfi is fixing up another bug in linkedin ads, and then he'll get this merged — I expect it's this or next week. |
@natikgadzhi , I saw that the fix has been merged. Solved merge conflicts and ready to merge again |
What
This PR aims to solve the issue (#49457). It does this quite simply by upgrading the linkedin ads version of the creatives stream to the
202410
version. This is an important step of the Linkedin ads API with its long term objective to deprecate the endpoints InMailContents API or the AdDirectSponsoredContents API.How
Upgrading the manifest.yaml file of the source connector.
Review guide
User Impact
User impacts only the creatives stream is only the addition of the name column. This can be confirmed here.
Can this PR be safely reverted and rolled back?