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

🎉 New source: Google Ads API #3842

Merged
merged 23 commits into from
Jun 14, 2021
Merged

🎉 New source: Google Ads API #3842

merged 23 commits into from
Jun 14, 2021

Conversation

harshithmullapudi
Copy link
Contributor

@harshithmullapudi harshithmullapudi commented Jun 3, 2021 •
edited by sherifnada
Loading

What

Google Ads connector with the new google ads query language

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

@harshithmullapudi harshithmullapudi force-pushed the harshith-google-ads branch 2 times, most recently from 7b128ca to 270eabf Compare June 3, 2021 17:24
sherifnada
sherifnada reviewed Jun 3, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

Looks great so far -- few small comments here and there. Also recommend adding unit tests

airbyte-integrations/connectors/source-google-ads/integration_tests/configured_catalog.json Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
@sherifnada sherifnada mentioned this pull request Jun 4, 2021
Closed
16 tasks
sherifnada
sherifnada suggested changes Jun 7, 2021
View reviewed changes
airbyte-integrations/connectors/source-google-ads/build.gradle Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/unit_tests/test_source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/unit_tests/test_google_ads.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/utils.py Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
...-integrations/connectors/source-google-ads/source_google_ads/schemas/ad_group_ad_report.json Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Outdated Show resolved Hide resolved
@harshithmullapudi harshithmullapudi force-pushed the harshith-google-ads branch 3 times, most recently from 49b19a9 to 6b4879a Compare June 7, 2021 04:49
@harshithmullapudi harshithmullapudi force-pushed the harshith-google-ads branch 2 times, most recently from 04039bc to 153e525 Compare June 7, 2021 04:52
sherifnada
sherifnada reviewed Jun 7, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

getting there! One question: how is rate limiting handled in the current implementation?

airbyte-integrations/connectors/source-google-ads/source_google_ads/spec.json Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/source.py Show resolved Hide resolved
...-integrations/connectors/source-google-ads/source_google_ads/schemas/ad_group_ad_report.json Outdated Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/source_google_ads/google_ads.py Show resolved Hide resolved
airbyte-integrations/connectors/source-google-ads/integration_tests/acceptance.py Outdated
This won't work until we get sample credentials
"""

# def test_incremental_sync():
Copy link
Contributor

Choose a reason for hiding this comment

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

are these tests passing locally for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@sherifnada sherifnada changed the title New source: Google Ads API 🎉 New source: Google Ads API Jun 8, 2021
@sherifnada sherifnada mentioned this pull request Jun 8, 2021
Closed
@zestyping
Copy link
Contributor

Following this PR with excitement!

@davinchia
Copy link
Contributor

davinchia commented Jun 14, 2021 •
edited by github-actions bot
Loading

/test connector=source-google-ads

Error: No ref found for: harshith-google-ads

@davinchia
Copy link
Contributor

davinchia commented Jun 14, 2021 •
edited by github-actions bot
Loading

/test connector=source-google-ads repo=Velocity-Engineering/airbyte

Error: No ref found for: harshith-google-ads

@davinchia davinchia merged commit bad2fb1 into airbytehq:master Jun 14, 2021
@rx007 rx007 mentioned this pull request Nov 30, 2023
Open
@svc-security-github-snyk svc-security-github-snyk mentioned this pull request Nov 30, 2023
Open
@whitedragon206 whitedragon206 mentioned this pull request Dec 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

@cgardens cgardens Awaiting requested review from cgardens

@michel-tricot michel-tricot Awaiting requested review from michel-tricot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@harshithmullapudi @zestyping @davinchia @sherifnada @marcosmarxm