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.

Sign up for GitHub

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

Jump to bottom

🐛 Source Mailchimp: use datacenter parameter from apikey info #4285

Merged
merged 9 commits into from
Jun 25, 2021
Merged

🐛 Source Mailchimp: use datacenter parameter from apikey info #4285

merged 9 commits into from
Jun 25, 2021

Conversation

marcosmarxm
Copy link
Member

@marcosmarxm marcosmarxm commented Jun 23, 2021 •
edited
Loading

What

Closes #4198
change base url to use datacenter info

More information is possible to look at the mailchimp3 python client

https://github.com/VingtCinq/python-mailchimp/blob/d454e6280e1c66cfdbab502a11cb41f023ca0867/mailchimp3/mailchimpclient.py#L88-L89

How

update the base_url in init class instance to use datacenter info from apikey
https://mailchimp.com/developer/marketing/docs/export-api/#api-parameters

Recommended reading order

  1. source.py => added datacenter in the Authentication class isn't the best place... let me know if a need to decode in the Stream class instead;
  2. streams.py => update class variable to use datacenter variable

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in output spec
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste 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.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Documentation updated
    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
    • Build status added to build page
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

Connector Generator checklist

  • 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.

@github-actions github-actions bot added the area/connectors Connector related issues label Jun 23, 2021
@marcosmarxm
Copy link
Member Author

image

@marcosmarxm marcosmarxm requested a review from sherifnada June 23, 2021 02:17
@marcosmarxm
Copy link
Member Author

marcosmarxm commented Jun 23, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mailchimp

🕑 connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/962810132
✅ connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/962810132

sherifnada
sherifnada approved these changes Jun 23, 2021
View reviewed changes
airbyte-integrations/connectors/source-mailchimp/source_mailchimp/streams.py Outdated
primary_key = "id"
page_size = 100

def __init__(self, **kwargs):
super().__init__(**kwargs)
self.current_offset = 0
dc = kwargs['authenticator'].datacenter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dc = kwargs['authenticator'].datacenter
data_center = kwargs['authenticator'].datacenter

airbyte-integrations/connectors/source-mailchimp/source_mailchimp/streams.py Outdated
@@ -33,13 +33,15 @@


class MailChimpStream(HttpStream, ABC):
url_base = "https://us2.api.mailchimp.com/3.0/"
url_base = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use a dynamic property

Suggested change
url_base = ""
@property
def url_base(self) -> str:
return f"https://{self.data_center}.api.mailchimp.com/3.0/"

airbyte-integrations/connectors/source-mailchimp/source_mailchimp/source.py Outdated
@@ -37,6 +37,7 @@

class HttpBasicAuthenticator(TokenAuthenticator):
def __init__(self, auth: Tuple[str, str], auth_method: str = "Basic", **kwargs):
self.datacenter = auth[1].split('-').pop()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.datacenter = auth[1].split('-').pop()
# API keys have the format <key>-<data_center>. See https://mailchimp.com/developer/marketing/docs/fundamentals/#api-structure
self.datacenter = auth[1].split('-').pop()

@marcosmarxm
Copy link
Member Author

marcosmarxm commented Jun 23, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mailchimp

🕑 connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/964132350
✅ connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/964132350

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 23, 2021
@marcosmarxm
Copy link
Member Author

marcosmarxm commented Jun 23, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mailchimp

🕑 connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/964476095
✅ connectors/source-mailchimp https://github.com/airbytehq/airbyte/actions/runs/964476095

@marcosmarxm marcosmarxm force-pushed the marcosmarxm/mailchimp-datacenter-url branch from 3be5792 to 6e8e9ac Compare June 24, 2021 22:58
@marcosmarxm marcosmarxm merged commit 77f7096 into master Jun 25, 2021
@marcosmarxm marcosmarxm deleted the marcosmarxm/mailchimp-datacenter-url branch June 25, 2021 00:55
vovavovavovavova
vovavovavovavova reviewed Jul 2, 2021
View reviewed changes
airbyte-integrations/connectors/source-mailchimp/source_mailchimp/streams.py Outdated

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

this removal breaks reading logic and acceptance tests, related issue #4499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vovavovavovavova vovavovavovavova vovavovavovavova left review comments

@sherifnada sherifnada sherifnada approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source Mailchimp: add parameter datacenter
3 participants
@marcosmarxm @sherifnada @vovavovavovavova