-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 Github: IssueReactions - re-implemented using GraphQL #21457
Conversation
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
/test connector=connectors/source-github
Build FailedTest summary info:
|
Signed-off-by: Sergey Chvalyuk <[email protected]>
/test connector=connectors/source-github
Build FailedTest summary info:
|
/test connector=connectors/source-github
Build PassedTest summary info:
|
/test connector=connectors/source-github
Build FailedTest 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.
Awesome work - one question below about the state format change
issue_reactions: ["airbytehq/integration-test", "11", "created_at"] | ||
issue_reactions: ["airbytehq/integration-test", "created_at"] |
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.
Does this change imply that the format of state changed for an incremental stream? If so, that's probably a major (breaking) semver change
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.
yes state format changed, state object simplified
Can you advice how to be in such cases? Are you proposing to increase version number 0.3.11 -> 0.4.0?
Thank you
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.
To answer the question, the user experience needs to be considered:
- If the sync will continue as it has in the past (maintain it's incremental position) - patch change (no user intervention needeed)
- If the sync will succeed, but require a full import again because the state format is changed - minor change (no user intervention needed, but a slow sync will happen)
- If the sync will fail until the user takes action or the records returned are changed - major change.
I think we are in category 2, like you suggest - is that correct?
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.
answer - 2
because the state format is changed
it will re-read all issue-reaction records again from "start_date"
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! Then as you suggest, this should be v0.4.0 of the connector.
We are trying to get good at semver versioning of connectors from the user's point of view so we can automate warnings and breaking change announcements in the app.
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.
I have bumped to 0.4.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.
@evantahler can you please look can we proceed with this PR?
Signed-off-by: Sergey Chvalyuk <[email protected]>
Signed-off-by: Sergey Chvalyuk <[email protected]>
/test connector=connectors/source-github
Build FailedTest summary info:
|
Signed-off-by: Sergey Chvalyuk <[email protected]>
/test connector=connectors/source-github
Build PassedTest summary info:
|
/test connector=connectors/source-github
Build PassedTest summary info:
|
/publish connector=connectors/source-github
if you have connectors that successfully published but failed definition generation, follow step 4 here |
Airbyte Code Coverage
|
Signed-off-by: Sergey Chvalyuk [email protected]
What
Trying to partially improve this oncall https://github.com/airbytehq/oncall/issues/1303
Performance for stream IssueReactions
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described here