TDL-26228 New Stream addition #95

somethingmorerelevant · 2024-07-19T08:37:20Z

Description of change

Adds Support for the following streams
- Describe Leads (Provides Metadata about the lead's fields)
  - Type: Full Table
  - Primary Keys: id
- Program Tags (Provides tags associated with a program)
  - Type: Full Table
  - Primary Keys: None
  - Parent Child Hierarchy with program's stream
  - Implements activate version messages.
- Tag Types (Provides Master table for tags)
  - Type: Full Table
  - Primary Keys: None
  - Implements activate version messages.

Manual QA steps

Execute Sync and Discovery mode locally.

Risks

Low (Since the changes have been tested through PR Alpha)

Rollback steps

revert this branch

RushiT0122 · 2024-07-29T14:49:58Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog

+## 2.6.2


It should be minor version.

tap_marketo/sync.py

RushiT0122 · 2024-07-29T15:12:34Z

tap_marketo/sync.py

+    if stream_version is None:
+        stream_version = int(time.time() * 1000)
+        singer.log_info("Using new activate version %s", stream_version)
+    else:
+        singer.log_info("Using interupted activate version %s", stream_version)
+    return stream_version


Suggested change

if stream_version is None:

stream_version = int(time.time() * 1000)

singer.log_info("Using new activate version %s", stream_version)

else:

singer.log_info("Using interupted activate version %s", stream_version)

return stream_version

if stream_version:

singer.log_info("Using interupted activate version %s", stream_version)

else:

stream_version = int(time.time() * 1000)

singer.log_info("Using new activate version %s", stream_version)

return stream_version

since this condition is simplified and the changes are tested and validated, changing the sequence of the condition will not have any significant impact.
i would suggest against this change, unless this implementation is causing any adverse impact.

RushiT0122 · 2024-07-29T15:18:07Z

tap_marketo/sync.py

+        data = client.request("GET", endpoint, endpoint_name="programs", params=params)
+        if "warnings" in data and NO_ASSET_MSG in data["warnings"]:
+            break
+        if "errors" in data and data["errors"] != []:


Any specific reason, we are checking data["errors"] != []? Can we use simplified condition like below,

Suggested change

if "errors" in data and data["errors"] != []:

if "errors" in data and data["errors"]:

RushiT0122 · 2024-07-29T15:19:35Z

tap_marketo/sync.py

+        if "warnings" in data and NO_ASSET_MSG in data["warnings"]:
+            break


Should we log these warnings as well like errors?

Same suggestion on L#581-L#582.

those are not actual warnings, it just means that we have reached the end of the list and there are no more objects to iterate.

RushiT0122 · 2024-07-29T15:24:49Z

tap_marketo/sync.py

+
+def sync_program_tags(client, state, stream):
+    singer.write_schema(stream["tap_stream_id"], stream["schema"], stream["key_properties"])
+    first_run = check_if_first_sync(state, stream["tap_stream_id"])


To clearly convey that this is boolean variable, we should use name like is_first_run or is_initail_sync.

Same suggestion on L#560.

RushiT0122 · 2024-07-29T15:35:09Z

tap_marketo/sync.py

+    if last_synced_program:
+        for indx, prog in enumerate(program_ids):
+            if prog == last_synced_program:
+                start = indx
+                singer.log_info("Last Sync was interupted at index: %s program: %s",indx, prog)


Suggested change

if last_synced_program:

for indx, prog in enumerate(program_ids):

if prog == last_synced_program:

start = indx

singer.log_info("Last Sync was interupted at index: %s program: %s",indx, prog)

if last_synced_program_id:

for index, program_id in enumerate(program_ids):

if program_id == last_synced_program_id:

start = index

singer.log_info("Last Sync was interupted at index: %s program: %s", index, program_id)

RushiT0122

Provided some suggestions in-line. In addtion to that please capitalise the new comments and docstrings added.

cosimon · 2024-07-31T14:47:46Z

tap_marketo/discover.py

+        if key_props:
+            mdata = metadata.write(mdata, (), 'table-key-properties', key_props)
+        else:
+            mdata = metadata.write(mdata, (), 'table-key-properties', [])


Suggested change

if key_props:

mdata = metadata.write(mdata, (), 'table-key-properties', key_props)

else:

mdata = metadata.write(mdata, (), 'table-key-properties', [])

if not key_props:

key_props = []

mdata = metadata.write(mdata, (), 'table-key-properties', key_props)

cosimon · 2024-07-31T14:52:24Z

tap_marketo/sync.py

+    while True:
+        data = client.request("GET", endpoint, endpoint_name="programs", params=params)
+        if "warnings" in data and NO_ASSET_MSG in data["warnings"]:
+            break


Avoid using while True with a break, instead create a has_more_data flag and iterate using that.

cosimon · 2024-07-31T14:53:47Z

tap_marketo/sync.py

+    """
+        checks if the sync was interupted in-between an extraction
+        if interupted returns the previous activate version number used
+        else returns a new activate version
+    """


This docstring appears to be wrong

cosimon · 2024-07-31T14:55:49Z

tap_marketo/sync.py

+        state = bookmarks.write_bookmark(state, stream["tap_stream_id"],INITAL_SYNC_KEY, True)
+        singer.write_state(state)
+
+    state = bookmarks.write_bookmark(state, stream["tap_stream_id"], ACTIVATE_VERSION_KEY, stream_version)
+    singer.write_state(state)


We do not need to be writing state twice consecutively here

Not necessary, since both statements are consecutive.
However this block will be executed only once (i.e the first sync), hence i made the state to be explicitly written in the same block.

cosimon · 2024-07-31T14:57:51Z

tap_marketo/sync.py

+    while True:
+        data = client.request("GET", endpoint, params=params)
+        if "warnings" in data and NO_ASSET_MSG in data["warnings"]:
+            break


Again avoid using while True with a break

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

…-26228

somethingmorerelevant added 15 commits July 16, 2024 10:47

added lead_describe, program_tags, tag_types impl

96d2297

update write_schema for lead_describe

2920ba2

updated leads describe schema

6f230a2

patched schema for leads_describe

fab192b

updated schema def

5e20d77

updated repl method

4d3a537

added repl method code

ec261a0

added repl method to metadata

ab3f436

added forced repl method

01c06ec

added activate version impl

8653530

updated sync impl

341ad3c

added schema message bef actv. version

9d736c6

updated impl and schema

e066b5a

added stream impl

63e4d6e

update changelog

232627f

somethingmorerelevant marked this pull request as ready for review July 22, 2024 08:17

somethingmorerelevant added 2 commits July 22, 2024 08:55

added eof

625c626

added new test scenario

a0c239f

somethingmorerelevant requested review from cosimon, RushiT0122 and sgandhi1311 July 25, 2024 10:04

RushiT0122 reviewed Jul 29, 2024

View reviewed changes

CHANGELOG.md Outdated

@@ -1,5 +1,8 @@

# Changelog

## 2.6.2

Copy link

RushiT0122 Jul 29, 2024

Choose a reason for hiding this comment

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

It should be minor version.

RushiT0122 reviewed Jul 29, 2024

View reviewed changes

tap_marketo/sync.py Outdated Show resolved Hide resolved

RushiT0122 reviewed Jul 29, 2024

View reviewed changes

RushiT0122 requested changes Jul 29, 2024

View reviewed changes

cosimon requested changes Jul 31, 2024

View reviewed changes

somethingmorerelevant and others added 12 commits August 1, 2024 00:48

Update tap_marketo/sync.py

82947be

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

added new fields

5abfc2f

fixed review comments

76dafa5

Merge branch 'TDL-26228' of github.com:singer-io/tap-marketo into TDL…

c31378b

…-26228

fixed issue with program_tags

141cb40

fixed tag_types issue

4a4316c

updated schema for programs

d8f5ff7

* Add new field to activitiy streams

7ef5391

* Update tests

ccbef1c

Merge branch 'master' into TDL-26228

8096123

updated sync max_export to support float

d8ecfc9

Merge branch 'TDL-26228' of github.com:singer-io/tap-marketo into TDL…

3688203

…-26228

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-26228 New Stream addition #95

TDL-26228 New Stream addition #95

somethingmorerelevant commented Jul 19, 2024

RushiT0122 Jul 29, 2024

RushiT0122 Jul 29, 2024

somethingmorerelevant Jul 30, 2024

RushiT0122 Jul 29, 2024

RushiT0122 Jul 29, 2024

RushiT0122 Jul 29, 2024

somethingmorerelevant Jul 30, 2024

RushiT0122 Jul 29, 2024

RushiT0122 Jul 29, 2024 •

edited

Loading

RushiT0122 Jul 29, 2024

RushiT0122 left a comment

cosimon Jul 31, 2024

cosimon Jul 31, 2024

cosimon Jul 31, 2024

cosimon Jul 31, 2024

somethingmorerelevant Aug 2, 2024

cosimon Jul 31, 2024

	if "errors" in data and data["errors"] != []:
	if "errors" in data and data["errors"]:

		if "warnings" in data and NO_ASSET_MSG in data["warnings"]:
		break

TDL-26228 New Stream addition #95

Are you sure you want to change the base?

TDL-26228 New Stream addition #95

Conversation

somethingmorerelevant commented Jul 19, 2024

Description of change

Manual QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushiT0122 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushiT0122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushiT0122 Jul 29, 2024 •

edited

Loading