Skip to content

🐛 Source Slack: Fix reading threads issue #4860

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
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
Contributor

@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
Contributor 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
@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
Contributor Author

lazebnyi commented Jul 21, 2021

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 lazebnyi requested a review from yaroslav-dudar July 21, 2021 18:10
@lazebnyi
Copy link
Contributor 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
Contributor 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
self.set_sub_primary_key()
super().__init__(**kwargs)

def set_sub_primary_key(self):
Copy link
Contributor

@sherifnada sherifnada Jul 22, 2021

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
Contributor Author

@lazebnyi lazebnyi Jul 22, 2021

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
Contributor 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
Contributor 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
Contributor 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

Labels

area/connectors Connector related issues area/documentation Improvements or additions to documentation

7 participants

@lazebnyi @midavadim @yaroslav-dudar @sherifnada @antixar @bazarnov