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

New Source Salesforce Marketingcloud Singer #10026

Closed
wants to merge 9 commits into from

Conversation

schlattk
Copy link
Contributor

@schlattk schlattk commented Feb 3, 2022

What

Adding a new source for salesforce marketing cloud utilising an existing singer source. This connector has already been running successfully for several months
(this is a cleaned up version of #9080)

How

Using an existing Singer source (tap-exacttarget).
https://github.com/singer-io/tap-exacttarget

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.

New Connector

Community member or Airbyter

  • Community member? 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
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • 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 connector is published, connector added to connector index 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

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
Screenshot 2021-12-23 at 10 59 58

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

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

Screenshot 2021-12-23 at 10 59 58

@github-actions github-actions bot added the area/connectors Connector related issues label Feb 3, 2022
@schlattk schlattk changed the title Mc singer source squashed New Source Salesforce Marketingcloud Singer Feb 3, 2022
@harshithmullapudi
Copy link
Contributor

Thanks a lot. I will check with the team and credentials and merge it

@igrankova
Copy link
Contributor

Hi, @harshithmullapudi, I'm trying to get the CI creds for the Marketing Cloud, but received the answer from the support: Marketing Cloud does not have a sandbox environment like Salesforce.
I asked @schlattk for help in one of the connected issues where he told that maybe he can help with the account (#4933) and waiting for the response.
For now we only have dev and sandbox accounts, but we can't access Marketing Cloud from those accounts. I tried to investigate whether I can use API endpoints, but I can't without the account.

@harshithmullapudi
Copy link
Contributor

Thanks for the update @igrankova.

@harshithmullapudi harshithmullapudi added the waiting-for-cred Waiting for CI credentials label Mar 1, 2022
@joaquin-orono
Copy link

Hello, i have an account to validate salesforce marketing, the credentials are from one customer of mine so i have to check by myself because i can't share with nobody, how can I use this connector to test?

@harshithmullapudi
Copy link
Contributor

Hey @joaquin-orono you can also fork @schlattk and use it.

@igrankova is there any update on this?

@igrankova
Copy link
Contributor

@igrankova is there any update on this?
Now Andy is negotiating with SFMC according the discount for a sandbox account, still in progress.

@notsaman
Copy link

Big fan of this, cheers. The fork worked well enough for me, so would be cool to see this in an official release.

@igrankova
Copy link
Contributor

@harshithmullapudi, today we have a call with FSCM team about the sandbox.

@joaquin-orono
Copy link

Hello @schlattk! Quick question, as i saw the connector is working, if I follow your instructions https://github.com/schlattk/airbyte/tree/mc_singer_source_squashed/airbyte-integrations/connectors/source-sf-marketingcloud-singer
It can work in my environment, i have airbyte 0.36.6-alpha?

Thanks a lot!

@schlattk
Copy link
Contributor Author

schlattk commented May 3, 2022 via email

@joaquin-orono
Copy link

Hello @schlattk, sorry for my unknowledge about how to make run salesforce marketing connector, maybe i have not all knowledges to make it run, can you explain a bit more the steps before to the requirements, i'm trying to clone the url shared before but i get an error, maybe i'm doing something wrong. Thanks a lot!

@schlattk
Copy link
Contributor Author

schlattk commented May 5, 2022

Hi @joaquin-orono If you fork the repo and follow the instructions in the readme (of sf-marketingcloud-singer source) and run docker you should be able to see this source as one of your connectors.

@CLAassistant
Copy link

CLAassistant commented May 5, 2022

CLA assistant check
All committers have signed the CLA.

@igrankova
Copy link
Contributor

Please, find creds for the SFMC sandbox in the LastPass: Salesforce Marketing Cloud sandbox

@harshithmullapudi
Copy link
Contributor

@irynakruk were you able to open Installed Packages? I need to create client_id and client_secret https://developer.salesforce.com/docs/marketing/marketing-cloud/guide/mc-dev-setup.html

@igrankova
Copy link
Contributor

@irynakruk were you able to open Installed Packages? I need to create client_id and client_secret https://developer.salesforce.com/docs/marketing/marketing-cloud/guide/mc-dev-setup.html

Hi, @harshithmullapudi, I see that I can't login anymore, it looks like my verification app is not connected to the system and the codes are not valid anymore so I can't look to this problem. Maybe we can manage a call or something to find the way to solve this.

@harshithmullapudi
Copy link
Contributor

@igrankova I installed the app on my mobile to log in and check. I am uninstalling it now can you check if you can install i.

@igrankova
Copy link
Contributor

igrankova commented May 24, 2022

@harshithmullapudi I've reinstalled the app. I'm in SFMC, looking to our issue. I'll let you know.

@igrankova
Copy link
Contributor

@harshithmullapudi I've created a package Airbyte Cloud with read scopes.
image
Unfortunately it doesn't support local redirect URIs, but all the https URIs I've added. Let me know if I can do something else for you.
I guess you'll have to reinstall your authentication app once again to get the access to the instance.

@alafanechere alafanechere removed the waiting-for-cred Waiting for CI credentials label Jun 2, 2022
@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Jul 15, 2022

/test connector=connectors/source-sf-marketingcloud-singer

🕑 connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2675147226
❌ connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2675147226
🐛 https://gradle.com/s/web2vbhcjep5q

Build Failed

Test summary info:

Could not find result summary

@harshithmullapudi
Copy link
Contributor

Screenshot 2022-07-15 at 1 44 30 PM

@schlattk I am getting this error while running tests locally with 3.9 version of python. Is there some fix?

@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Jul 15, 2022

/test connector=connectors/source-sf-marketingcloud-singer

🕑 connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2675978074
❌ connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2675978074
🐛 https://gradle.com/s/fzldydon5ij5i

Build Failed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestBasicRead.test_read because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
============================== 3 skipped in 0.66s ==============================

@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Jul 15, 2022

/test connector=connectors/source-sf-marketingcloud-singer

🕑 connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676031945
❌ connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676031945
🐛 https://gradle.com/s/ss2x6eb5jwqvw

Build Failed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestBasicRead.test_read because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
============================== 3 skipped in 0.68s ==============================

@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Jul 15, 2022

/test connector=connectors/source-sf-marketingcloud-singer

🕑 connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676090952
❌ connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676090952
🐛 https://gradle.com/s/bmymmyqaypog2

Build Failed

Test summary info:

=========================== short test summary info ============================
ERROR test_core.py::TestSpec::test_config_match_spec[inputs0] - FileNotFoundE...
ERROR test_core.py::TestConnection::test_check[inputs0] - FileNotFoundError: ...
ERROR test_core.py::TestDiscovery::test_discover[inputs0] - FileNotFoundError...
ERROR test_core.py::TestDiscovery::test_defined_cursors_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_refs_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-allOf]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-not]
ERROR test_core.py::TestDiscovery::test_primary_keys_exist_in_schema[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestBasicRead.test_read because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
==================== 9 passed, 3 skipped, 8 errors in 3.29s ====================

@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Jul 15, 2022

/test connector=connectors/source-sf-marketingcloud-singer

🕑 connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676153872
❌ connectors/source-sf-marketingcloud-singer https://github.com/airbytehq/airbyte/actions/runs/2676153872
🐛 https://gradle.com/s/ttgtnebxdq5ju

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestConnection::test_check[inputs0] - docker.errors.Cont...
FAILED test_core.py::TestDiscovery::test_discover[inputs0] - docker.errors.Co...
ERROR test_core.py::TestDiscovery::test_defined_cursors_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_refs_exist_in_schema[inputs0]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-allOf]
ERROR test_core.py::TestDiscovery::test_defined_keyword_exist_in_schema[inputs0-not]
ERROR test_core.py::TestDiscovery::test_primary_keys_exist_in_schema[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestBasicRead.test_read because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestFullRefresh.test_sequential_reads because not found in the config
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
============== 2 failed, 10 passed, 3 skipped, 5 errors in 6.52s ===============

@schlattk
Copy link
Contributor Author

schlattk commented Jul 19, 2022

@harshithmullapudi
I get this error when I run tests locally
line 20, in <listcomp>\n return [str(get_value_by_dot_notation(config, key)) for key in secret_key_names if config.get(key)]\nAttributeError: 'ConfigContainer' object has no attribute 'get'"}}
It appears in this issue which has supposedly been resolved though #8710.

@harshithmullapudi
Copy link
Contributor

@schlattk Hey I have spent a lot of time trying to get the tests working but there are a lot of moving pieces. To run the tests we need to Make the whole source work with python 3.9 which we need to fix?

  1. Problem with suds-judo installation with 3.9 - Remove suds-jurko salesforce-marketingcloud/FuelSDK-Python#138
  2. Fixing that is we will also have to update the dependency in tap-exacttarget
  3. Even then we have a dependency in singer-helper json-schema which is colliding with the airbyte-cdk which can't be resolved.

Ideally solution i could see is to push the docker image and then add to out source_definitions

cc: @marcosmarxm @jerri-airbyte

@sajarin sajarin added internal and removed bounty bounty-M Maintainer program: claimable medium bounty PR labels Sep 26, 2022
@igrankova
Copy link
Contributor

igrankova commented Oct 11, 2022 via email

@igrankova
Copy link
Contributor

igrankova commented Oct 11, 2022 via email

@marcosmarxm
Copy link
Member

Hello 👋, first thank you for this amazing contribution.

We really appreciate the effort you've made to improve the project.
We ask you patience for the code review. Last month our team was focused on Hacktoberfest event and that probably left some PR without the proper feedback. And this week, due to the Thanksgiving US Holiday, most our team is out of office with their families. Another important piece of information why code won't be merge this week is: as a safety measure the core team has decided to freeze merging code to main branch to keep the release stable. Next week we'll return to you with the proper code review and update the status of your contribution.

If you have any questions feel free to send me a message in Slack!
Thanks!

@marcosmarxm
Copy link
Member

Hello 👋:skin-tone-2: and thank you for your contribution!

Airbyte has instituted a code freeze between 19 and 30 December, to make sure there are no disruptions during the holidays.
Because of this, reviewing and merging your contribution may take longer than usual.
We apologize for the delay, but we want everyone to have a quiet and happy holiday.

If you have any questions or need further clarification, please don't hesitate to ping via Slack.

@sherifnada
Copy link
Contributor

closing since we are no longer accepting Singer-based connectors into the Airbyte monorepo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.