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.

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

Tdl 17006 revert back companies to incremental #44

Merged

Conversation

savan-chovatiya
Copy link
Contributor

@savan-chovatiya savan-chovatiya commented Dec 30, 2021

Description of change

TDl-17006: Revert back the companies stream to work as an INCREMENTAL stream

  • Reverted back companies stream to incremental again same as v1.1.3.

NOTE: Bookmark integration test was failing for companies after changing it to incremental. It got the same records for both syncs in the bookmark test but we don't have a token used in CIrcleCI to validate possible dates to check that test.
For passing build, added companies to untestable_stream and created a backlog card TDL-17035 for the fix. (Slack Thread)

Manual QA steps

  • Verified that companies stream is marked as incremental in catalog with valid replication key and inclusion.
  • Verified that a number of records differ as per start_date/bookmark and also respect bookmark/start_date.

Risks

Rollback steps

  • revert this branch

# Stream `companies` is changed from incremental to full_table
# so adding replication key used at incremental time to keep consistency.
replication_key = 'updated_at' if stream == "companies" else STREAMS[stream].replication_key
replication_key = STREAMS[stream].replication_key
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The companies stream is changed to incremental again so it has replication_key now so no need to handle it specifically.

Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

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

I looks like the integration test is failing becuase the tap isn't using the bookmark on the second sync.

======================================================================
FAIL: test_run (test_intercom_bookmarks.IntercomBookmarks) (stream='companies')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/tests/test_intercom_bookmarks.py", line 187, in test_run
    self.assertLess(second_sync_count, first_sync_count)
AssertionError: 162 not less than 162

@savan-chovatiya
Copy link
Contributor Author

I looks like the integration test is failing becuase the tap isn't using the bookmark on the second sync.

======================================================================
FAIL: test_run (test_intercom_bookmarks.IntercomBookmarks) (stream='companies')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/tests/test_intercom_bookmarks.py", line 187, in test_run
    self.assertLess(second_sync_count, first_sync_count)
AssertionError: 162 not less than 162

For passing build, added companies to untestable stream and created backlog card to fix it as per discussion in this Slack Thread.

Copy link
Contributor

@dmosorast dmosorast left a comment

Choose a reason for hiding this comment

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

Discussed offline, the test is manufacturing bookmarks, so the test failure is not necessarily a bug. Capturing the work, and functional testing should be sufficient. Approved ✅

@hpatel41 hpatel41 merged commit 3993fd7 into crest-master Jan 5, 2022
@hpatel41 hpatel41 mentioned this pull request Jan 5, 2022
KrisPersonal pushed a commit that referenced this pull request Jan 5, 2022
* TDL-17000: fix breaking changes and TDL-17017: Fix bookmark for conversation_parts stream. (#45)

* Reverted conversation_parts to full_table for parent as V1.1.3

* Updated bookmark test to handle conversation_parts

* Updated comments

* Added code to select parent_stream if only child stream is selected same as v1.1.3

* Added code comments

* Resolved review comment

* Resolved review comment

* Added code comments

* Added bookmark mechanism for conversation_parts same as before but independant of conversations

* Added code comments

* Added query paramater to get conversations greater and equal to bookmark

* Adding changes of PR42 for better unit tests

* Added logger messages

* Added unit test

* Updated unit test

* Updated unit test

* Updated variables in unit test

* Added time_extracted for conversation_parts

* Resolved review comments

* Added companies to untestable for bookmark test

* Reverted changes of last commit

* Tdl 17003 Added back missing field for contact streams (#43)

* Reverted the logic as per old version

* Removed leads as well as it is not supported by tap

* Tdl 17006 revert back companies to incremental (#44)

* Reverted companies to incremental stream

* Debuging integration test

* Removed print statement

* Added companies to untestable for bookmark test

* TDL-17002: Make the logger messages more descriptive (#46)

* added logger messages

* removed unnecessary loggers

* removed unnecessary loggers

* updated the code to use stream name instead of parent stream name

Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>
Co-authored-by: Umang Agrawal <80704207+umangagrawal-crest@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants