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 Zendesk Support: update cursor
for Tickets
stream
#28774
🚨🚨 Source Zendesk Support: update cursor
for Tickets
stream
#28774
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,
|
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ❌ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
cursor
for Tickets
stream
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ❌ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ❌ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ❌ |
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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ❌ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ❌ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
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-zendesk-support test
airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/spec.json
Outdated
Show resolved
Hide resolved
…ndesk-check-permissions # Conflicts: # airbyte-integrations/connectors/source-zendesk-support/Dockerfile # airbyte-integrations/connectors/source-zendesk-support/metadata.yaml # docs/integrations/sources/zendesk-support.md
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
☁️ 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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
☁️ 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-zendesk-support 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.
Nice work so far. I had a few questions to clarify how we decide state. Additionally, because this is a breaking change I want to make sure of a few things.
The new cursor value:
In your testing, did you confirm that the incremental/tickets/cursor.json
endpoint returns a field with generated_timestamp
? I saw it in the tests expected_records.jsonl
, but their documentation is quite confusing with a lot of ways to do things. The part that stuck out to me was:
Incremental Ticket Export, Cursor Based (What we use):
https://developer.zendesk.com/api-reference/ticketing/ticket-management/incremental_exports/#incremental-ticket-export-cursor-based
This doesn’t make mention of the generated_timestamp field in the response.
Incremental Ticket Export, Time Based:
https://developer.zendesk.com/api-reference/ticketing/ticket-management/incremental_exports/#incremental-ticket-export-time-based
This is the one that explicitly mentioned the generated_timestamp
field being present. Although in their response example it also does not include it.
Our code looks to be using the cursor based given how we paginate, but we should double check by making a CURL request that emulates our Ticket stream requests and verify that our expected cursor value generated_timestamp
is present.
Lastly, we have some merge conflicts that we should address before final approval.
new_value = (latest_record or {}).get(self.cursor_field, 0) | ||
return {self.cursor_field: max(new_value, old_value)} | ||
|
||
def check_stream_state(self, stream_state: Mapping[str, Any] = None): |
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.
Please add the return type in the signature
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.
added
if self.sideload_param: | ||
params["include"] = self.sideload_param | ||
if next_page_token: | ||
params.update(next_page_token) | ||
return params | ||
|
||
def get_updated_state(self, current_stream_state: MutableMapping[str, Any], latest_record: Mapping[str, Any]) -> Mapping[str, Any]: | ||
old_value = (current_stream_state or {}).get(self.cursor_field, 0) | ||
new_value = (latest_record or {}).get(self.cursor_field, 0) |
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.
Are we certain that records are transmitted in chronological order and there is no way to get records outside the current slice window? The past Salesforce bug has me cautious about this type of thing.
Is it valid for the stream_state and the latest_record to not have a generated_timestamp
value. It doesn't seem right to save a timestamp of 0
as state. That would effectively turn this into an incredibly early sync on the next run (unless we get the max of start_date and state). Regardless, if this condition shouldn't be possible, then it feels more like we should be throwing some type of error
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.
updated with start date
airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py
Show resolved
Hide resolved
# Conflicts: # airbyte-integrations/connectors/source-zendesk-support/Dockerfile # airbyte-integrations/connectors/source-zendesk-support/metadata.yaml # docs/integrations/sources/zendesk-support.md
@brianjlai ,
This situation was discovered during the incremental sync tests: after the initial sync and setting stream_state to the updated_at value of the latest record, we performed a new sync with start_date=stream_state_value in the query to ensure that no records would be returned. However, there were some records in the response, indicating that the filter works on the |
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
☁️ 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-zendesk-support test
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
Code format checks | ✅ |
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
☁️ 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-zendesk-support 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.
Thanks for rechecking the generated_timestamp
the changes make sense. Approved, but before we publish and merge, we should coordinate with @erica-airbyte to communicate the breaking change out. I see on the OC that it was to go out 8/10, but lets make sure we're aligned on a new date to release
airbyte-integrations/connectors/source-zendesk-support/source_zendesk_support/streams.py
Show resolved
Hide resolved
@brianjlai is this the exact same change it just didnt go out? |
@erica-airbyte , yes, this is the exact same change it just didnt go out on time |
source-zendesk-support test report (commit
|
Step | Result |
---|---|
Connector package install | ✅ |
Build source-zendesk-support docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate airbyte-integrations/connectors/source-zendesk-support/metadata.yaml | ✅ |
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-zendesk-support test
releases: | ||
breakingChanges: | ||
1.0.0: | ||
message: "`cursor_field` for `Tickets` stream is changed to `generated_timestamp`" | ||
upgradeDeadline: "2023-07-19" |
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 is a poor breaking change message. Why was this change? What do users need to do to get syncs working again?
What
Resolve https://github.com/airbytehq/oncall/issues/2593
How
cursor
togenerated_timestamp
forTickets
stream (revert 🎉 Source Zendesk: sync rate improvement #9456)Recommended reading order
streams.py
🚨 User Impact 🚨
cursor_field
forTickets
stream is changed togenerated_timestamp
. Need torefresh
source schema and make areset
.Playbook: https://docs.google.com/document/d/1yO70HQkV4Mk1SZriFrCSoynw7E2yz1Civ3KmFJHYNvw/edit
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.