Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 Source Slack: Fix reading threads issue #4860

Merged
merged 11 commits into from
Jul 23, 2021
Merged

🐛 Source Slack: Fix reading threads issue #4860

merged 11 commits into from
Jul 23, 2021

Conversation

lazebnyi
Copy link
Collaborator

@lazebnyi lazebnyi commented Jul 20, 2021 •
edited
Loading

What

#4680 - Source Slack: reading threads failed in stream slicing stage

How

  • Fixed typo in stream_slices method
  • Short code refactoring
  • Added filtering for threads from client-side
  • Added test for threads stream incremental sync

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Unit & integration tests added as appropriate (and are passing)
    ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
  • Code reviews completed
  • Documentation updated
    • README.md
    • Created or updated reference docs in docs/integrations/<source or destination>/<name>.
    • Changelog in the appropriate page in docs/integrations/.... See changelog example
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers

@lazebnyi lazebnyi linked an issue Jul 20, 2021 that may be closed by this pull request
Source Slack: problem reading threads #4680
Closed
@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jul 20, 2021
@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 20, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1049109485
✅ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1049109485

midavadim
midavadim suggested changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@midavadim midavadim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well ... we definitely need to run at least real sync for threads and messages streams to be sure that it works correctly.

The ideal result - is to have a related acceptance test

It is not clear how this error appears here and why it was not seen in the previous testing

docs/integrations/sources/slack.md Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-slack/source_slack/source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-slack/source_slack/source.py Show resolved Hide resolved
@lazebnyi lazebnyi requested a review from antixar July 21, 2021 06:48
@lazebnyi lazebnyi changed the title Fixed reading threads issue 🐛 Fixed reading threads issue Jul 21, 2021
@lazebnyi lazebnyi changed the title 🐛 Fixed reading threads issue 🐛 Source Slack: Fix reading threads issue Jul 21, 2021
@lazebnyi lazebnyi requested a review from midavadim July 21, 2021 11:43
@lazebnyi
Copy link
Collaborator Author

well ... we definitely need to run at least real sync for threads and messages streams to be sure that it works correctly.

The ideal result - is to have a related acceptance test

It is not clear how this error appears here and why it was not seen in the previous testing

Added acceptance test for threads stream

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 22, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1055763143
❌ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1055763143

@lazebnyi lazebnyi removed the request for review from annalvova05 July 22, 2021 11:19
@lazebnyi lazebnyi requested review from sherifnada and removed request for TymoshokDmytro and antixar July 22, 2021 11:19
@lazebnyi lazebnyi force-pushed the lazebnyi/4680-slack-reading-threads-issue branch from 6b4465f to 5aa096c Compare July 22, 2021 15:40
@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 22, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1056926183
❌ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1056926183

sherifnada
sherifnada approved these changes Jul 22, 2021
View reviewed changes
airbyte-integrations/connectors/source-slack/source_slack/source.py
super().__init__(**kwargs)

def set_sub_primary_key(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the benefit of setting these attributes? I think it's confusing for the developer because it adds a layer of indirection for accessing primary_key[0] and primary_key[1] without changing those properties/adding anything to them. Would prefer if we didn't do this and just accessed the values directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the bug is that contributor who made a typo didn't paste square brackets to get the list item. And if in code we minimize those accessing and add variables for those values. We can avoid those mistakes.

Yes, this solution adds some complexity to the code.

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 22, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1057271946
❌ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1057271946

@lazebnyi lazebnyi force-pushed the lazebnyi/4680-slack-reading-threads-issue branch from 016836b to bdf04b2 Compare July 22, 2021 20:02
@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 23, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1059018464
✅ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1059018464

@lazebnyi
Copy link
Collaborator Author

lazebnyi commented Jul 23, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-slack

🕑 connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1059086092
✅ connectors/source-slack https://github.com/airbytehq/airbyte/actions/runs/1059086092

@lazebnyi lazebnyi marked this pull request as ready for review July 23, 2021 09:22
@lazebnyi lazebnyi merged commit 8488e9f into master Jul 23, 2021
@lazebnyi lazebnyi deleted the lazebnyi/4680-slack-reading-threads-issue branch July 23, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antixar antixar antixar left review comments

@midavadim midavadim midavadim approved these changes

@yaroslav-dudar yaroslav-dudar yaroslav-dudar approved these changes

@sherifnada sherifnada sherifnada approved these changes

@bazarnov bazarnov bazarnov approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source Slack: problem reading threads
6 participants
@lazebnyi @midavadim @yaroslav-dudar @sherifnada @antixar @bazarnov