-
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 Bing Ads: remove primary key for the geographic performance reports #31432
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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,
|
source-bing-ads test report (commit
|
Step | Result |
---|---|
Build source-bing-ads docker image for platform(s) linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
Code format checks | ✅ |
Validate metadata for source-bing-ads | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ❌ |
QA checks | ❌ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-bing-ads test
…ytehq/airbyte into ddavydov/#3068-remove-pk-completely
source-bing-ads test report (commit
|
Step | Result |
---|---|
Build source-bing-ads docker image for platform(s) linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate metadata for source-bing-ads | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ❌ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-bing-ads test
source-bing-ads test report (commit
|
Step | Result |
---|---|
Build source-bing-ads docker image for platform(s) linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate metadata for source-bing-ads | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-bing-ads test
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.
@davydov-d
The code changes look good to me. But one thing that strikes me as a little bit of a concern is that we're rapidly incrementing the major version from 0.2.3 -> 1.0.0 -> 2.0.0. Given the context I have from the past 2 PRs, this latest one feels more like a bug fix that came from 1.0.0. Is there a reason why you opted for this breaking change designation? Is it so that its more clear that users will need to refresh their schemas?
My concern here is that the customer in question saw a degraded behavior on their sync using 1.0.0. And I'm not sure we have a good use case where we want to retain this major version if its basically not functional. And if that were the case, a normal patch fix to 1.0.1 should hopefully ensure users don't use the version with the defect?
I temporarily marked this a Request changes just to make sure we're all aligned with the version bump before we ship this. I could be missing context on why the major bump is necessary and would like to know more
@brianjlai yes, this is a bug fix for v1.0.0. And yes, I decided it should be a breaking change for users to refresh their schemas and reset the data. |
source-bing-ads test report (commit
|
Step | Result |
---|---|
Build source-bing-ads docker image for platform(s) linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate metadata for source-bing-ads | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-bing-ads test
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.
@davydov-d thanks for clarifying. I can see the reasoning behind going either way. But I think we're aligned on using the patch. Though it might be clearer in the short run with the breaking change, for customers that write in, we can tell them to reset. And in the long run its probably better for us not to have a major version that contains the regression in it. Especially for OSS users who can dictate the version being used.
…reports (airbytehq#31432) Co-authored-by: davydov-d <davydov-d@users.noreply.github.com>
…reports (airbytehq#31432) Co-authored-by: davydov-d <davydov-d@users.noreply.github.com>
What
https://github.com/airbytehq/oncall/issues/3068
In version 1.0.0 there was an attempt to remove the primary key from the geographic performance reports.
It wasn't successful because I missed that the mixin the reports are inherited from declares it's own primary key 😢
How
Override the primary key with
None
, add unit tests🚨 User Impact 🚨
Same as in the previous update - users will get rid of primary keys, will have more data and will need to reset the data + refresh the schemas