Skip to content

TDL-21232 fix replace error #22

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

Merged
merged 6 commits into from
May 11, 2023
Merged

Conversation

kethan1122
Copy link
Contributor

Description of change

Tap is throwing below error when user edits the connection page and doesn't configure the model_id field. Somehow the app is setting the model_id to None in config json when user edits the connection page.

2022-11-04 05:30:22,302Z    tap - CRITICAL replace() argument 2 must be str, not None
2022-11-04 05:30:22,302Z    tap - Traceback (most recent call last):
2022-11-04 05:30:22,302Z    tap -   File "/code/orchestrator/tap-env/bin/tap-impact", line 33, in <module>
2022-11-04 05:30:22,302Z    tap -     sys.exit(load_entry_point('tap-impact==1.0.0', 'console_scripts', 'tap-impact')())
2022-11-04 05:30:22,303Z    tap -   File "/code/orchestrator/tap-env/lib/python3.9/site-packages/singer/utils.py", line 229, in wrapped
2022-11-04 05:30:22,303Z    tap -     return fnc(*args, **kwargs)
2022-11-04 05:30:22,303Z    tap -   File "/code/orchestrator/tap-env/lib/python3.9/site-packages/tap_impact/__init__.py", line 47, in main
2022-11-04 05:30:22,303Z    tap -     sync(client=client,
2022-11-04 05:30:22,303Z    tap -   File "/code/orchestrator/tap-env/lib/python3.9/site-packages/tap_impact/sync.py", line 406, in sync
2022-11-04 05:30:22,303Z    tap -     total_records = sync_endpoint(
2022-11-04 05:30:22,303Z    tap -   File "/code/orchestrator/tap-env/lib/python3.9/site-packages/tap_impact/sync.py", line 313, in sync_endpoint
2022-11-04 05:30:22,303Z    tap -     child_path = child_endpoint_config.get(
2022-11-04 05:30:22,303Z    tap - TypeError: replace() argument 2 must be str, not None

Manual QA steps

Risks

  • Low

Rollback steps

  • revert this branch

@RushiT0122 RushiT0122 self-requested a review May 11, 2023 11:39
@@ -15,7 +15,7 @@
],
extras_require={
'dev': [
'ipdb==0.11',
'ipdb',
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to remove specific ipdb version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not compatible with latest setuptools version mentioned here. so cci has been failing from long example

@kethan1122 kethan1122 requested a review from rdeshmukh15 May 11, 2023 11:47
Comment on lines 290 to 296
model_id = config.get('model_id', '')
process_child = True
# conversion_paths endpoint requires model_id tap config param
if child_stream_name == 'conversion_paths' and model_id == '':
if child_stream_name == 'conversion_paths' and not model_id:
process_child = False
if process_child:
write_schema(catalog, child_stream_name)
Copy link

@RushiT0122 RushiT0122 May 11, 2023

Choose a reason for hiding this comment

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

Instead for setting defualt value to empty string at L#290, can we simply set it to None?

Suggested change
model_id = config.get('model_id', '')
process_child = True
# conversion_paths endpoint requires model_id tap config param
if child_stream_name == 'conversion_paths' and model_id == '':
if child_stream_name == 'conversion_paths' and not model_id:
process_child = False
if process_child:
write_schema(catalog, child_stream_name)
model_id = config.get('model_id')
process_child = True
# conversion_paths endpoint requires model_id tap config param
if child_stream_name == 'conversion_paths' and not model_id:
process_child = False
if process_child:
write_schema(catalog, child_stream_name)

@kethan1122 kethan1122 merged commit 888056c into master May 11, 2023
@kethan1122 kethan1122 deleted the TDL-21232-fix-replace-error branch May 11, 2023 12:10
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.

3 participants