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

🐛 Source Marketo: improve field conversion to conform schema #8483

Merged
merged 21 commits into from
Dec 15, 2021

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Dec 3, 2021

Signed-off-by: Sergey Chvalyuk [email protected]

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.

How

  1. def normalize_datetime - I propose make date-time normalization more explicit than just trim chars.

  2. MarketoExportBase.parse_response - improved to be more robust for cases like in (3).

3 . For new stream activities_new_lead we get different field names on discover and read steps WE NEED TO FIX IT!!!
Tthis happened because discover and read return different names ( which probaly need to be normalized to the same name)

  'API Method Name' -> 'a_p_i_method_name'
  'api method name' -> 'apimethodname'
  
  'Request Id' -> 'request_id'
  "request id" -> 'requestid'

  'Modifying User' -> 'modifying_user'
  'modifying user' -> 'modifyinguser'
  
  WE NEED TO FIX IT !!!!
  1. Current implementation of utils.clean_string pretty 'magic' and can give ugly-looking results:

    'Step ID' -> 'step_i_d'
    "Campaign Run ID" -> 'campaign_run_i_d'
    "SFDC Type" -> "s_f_d_c_type"
    "Webpage ID" -> "webpage_i_d"

    but we can live with it

I think we can to get rid of utils.clean_string and just use s.lower().replace(' ', '_')

Orig Value                         utils.clean_string            s.lower().replace(' ', '_')
-----------------------------------------------------------------------------------------------
Add to List                      addto_list                      add_to_list
Add to Nurture                   addto_nurture                   add_to_nurture
Add to Opportunity               addto_opportunity               add_to_opportunity
Change Status in Progression     change_statusin_progression     change_status_in_progression
Merged in Sales                  mergedin_sales                  merged_in_sales
Push Lead to Marketo             push_leadto_marketo             push_lead_to_marketo
Received by                      receivedby                      received_by
Received Forward to Friend Email received_forwardto_friend_email received_forward_to_friend_email
Remove from List                 removefrom_list                 remove_from_list
Remove from Opportunity          removefrom_opportunity          remove_from_opportunity
Reply to Sales Email             replyto_sales_email             reply_to_sales_email
Sent by                          sentby                          sent_by
Sent Forward to Friend Email     sent_forwardto_friend_email     sent_forward_to_friend_email

Recommended reading order

  1. x.java
  2. y.python

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the new connector version is published, connector version bumped in the seed directory as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

@github-actions github-actions bot added the area/connectors Connector related issues label Dec 3, 2021
Signed-off-by: Sergey Chvalyuk <[email protected]>
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 3, 2021
@grubberr grubberr temporarily deployed to more-secrets December 3, 2021 11:40 Inactive
@grubberr grubberr self-assigned this Dec 3, 2021
@grubberr grubberr temporarily deployed to more-secrets December 3, 2021 14:32 Inactive
@lazebnyi lazebnyi linked an issue Dec 3, 2021 that may be closed by this pull request
@grubberr grubberr temporarily deployed to more-secrets December 4, 2021 20:05 Inactive
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr temporarily deployed to more-secrets December 4, 2021 20:11 Inactive
@grubberr
Copy link
Contributor Author

grubberr commented Dec 4, 2021

/test connector=connectors/source-marketo

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1539667649
✅ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1539667649
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        76      8    89%
	 source_acceptance_test/conftest.py                     109    109     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              235     95    60%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  54     24    56%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  946    442    53%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                         Stmts   Miss  Cover
	 ------------------------------------------------
	 source_marketo/__init__.py       2      0   100%
	 source_marketo/source.py       254    165    35%
	 source_marketo/utils.py         33      8    76%
	 ------------------------------------------------
	 TOTAL                          289    173    40%

@jrhizor jrhizor temporarily deployed to more-secrets December 4, 2021 20:31 Inactive
@grubberr grubberr temporarily deployed to more-secrets December 7, 2021 11:54 Inactive
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr temporarily deployed to more-secrets December 8, 2021 07:11 Inactive
@grubberr grubberr temporarily deployed to more-secrets December 8, 2021 07:33 Inactive
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr temporarily deployed to more-secrets December 8, 2021 07:35 Inactive
@grubberr
Copy link
Contributor Author

grubberr commented Dec 8, 2021

/test connector=connectors/source-marketo

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1552994430
✅ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1552994430
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        76      8    89%
	 source_acceptance_test/conftest.py                     109    109     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              235     95    60%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  54     24    56%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  946    442    53%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                         Stmts   Miss  Cover
	 ------------------------------------------------
	 source_marketo/__init__.py       2      0   100%
	 source_marketo/source.py       250    161    36%
	 source_marketo/utils.py         33      8    76%
	 ------------------------------------------------
	 TOTAL                          285    169    41%

@jrhizor jrhizor temporarily deployed to more-secrets December 8, 2021 07:40 Inactive
@grubberr grubberr temporarily deployed to more-secrets December 9, 2021 12:04 Inactive
@@ -37,8 +37,15 @@ def clean_string(string: str) -> str:
"updatedat" -> "updatedat"
"""

abbreviations = ("URL", "GUID", "IP")
if any(map(lambda w: w in string, abbreviations)):
fix = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added more cases for this unit_test 'test_clean_string'

@grubberr grubberr temporarily deployed to more-secrets December 13, 2021 09:05 Inactive
Copy link
Contributor

@antixar antixar left a comment

Choose a reason for hiding this comment

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

LGTM!
Please publish your changes

@grubberr grubberr temporarily deployed to more-secrets December 15, 2021 10:05 Inactive
@grubberr
Copy link
Contributor Author

grubberr commented Dec 15, 2021

/test connector=connectors/source-marketo

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582183707
❌ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582183707
🐛

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582183707
❌ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582183707
🐛

@jrhizor jrhizor temporarily deployed to more-secrets December 15, 2021 10:09 Inactive
Signed-off-by: Sergey Chvalyuk <[email protected]>
@grubberr grubberr temporarily deployed to more-secrets December 15, 2021 10:23 Inactive
@grubberr
Copy link
Contributor Author

grubberr commented Dec 15, 2021

/test connector=connectors/source-marketo

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582266662
✅ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582266662
Python tests coverage:

	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                                                 Stmts   Miss  Cover
	 ------------------------------------------------------------------------
	 source_acceptance_test/__init__.py                       2      0   100%
	 source_acceptance_test/base.py                          10      4    60%
	 source_acceptance_test/config.py                        76      8    89%
	 source_acceptance_test/conftest.py                     109    109     0%
	 source_acceptance_test/plugin.py                        47     47     0%
	 source_acceptance_test/tests/__init__.py                 4      0   100%
	 source_acceptance_test/tests/test_core.py              235     95    60%
	 source_acceptance_test/tests/test_full_refresh.py       38     27    29%
	 source_acceptance_test/tests/test_incremental.py        69     38    45%
	 source_acceptance_test/utils/__init__.py                 6      0   100%
	 source_acceptance_test/utils/asserts.py                 37      2    95%
	 source_acceptance_test/utils/common.py                  54     24    56%
	 source_acceptance_test/utils/compare.py                 62     25    60%
	 source_acceptance_test/utils/connector_runner.py        82     49    40%
	 source_acceptance_test/utils/json_schema_helper.py     115     14    88%
	 ------------------------------------------------------------------------
	 TOTAL                                                  946    442    53%
	 ---------- coverage: platform linux, python 3.8.10-final-0 -----------
	 Name                         Stmts   Miss  Cover
	 ------------------------------------------------
	 source_marketo/__init__.py       2      0   100%
	 source_marketo/source.py       250    161    36%
	 source_marketo/utils.py         33      8    76%
	 ------------------------------------------------
	 TOTAL                          285    169    41%

@jrhizor jrhizor temporarily deployed to more-secrets December 15, 2021 10:31 Inactive
@grubberr
Copy link
Contributor Author

grubberr commented Dec 15, 2021

/publish connector=connectors/source-marketo

🕑 connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582602615
✅ connectors/source-marketo https://github.com/airbytehq/airbyte/actions/runs/1582602615

@jrhizor jrhizor temporarily deployed to more-secrets December 15, 2021 12:05 Inactive
@grubberr grubberr temporarily deployed to more-secrets December 15, 2021 15:04 Inactive
@grubberr grubberr merged commit b7b7886 into master Dec 15, 2021
@grubberr grubberr deleted the grubberr/7776-source-marketo branch December 15, 2021 15:06
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
…hq#8483)

* normalize_datetime added, better parse_response

* clean_string updated

* get_json_schema updated

* bump version 0.1.1 -> 0.1.2

Signed-off-by: Sergey Chvalyuk <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix BigQuery destination normalization to accept any date-time format.
5 participants