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

adopt connector best practices in google ads source #1568

Closed
5 tasks
sherifnada opened this issue Jan 7, 2021 · 1 comment · Fixed by #1726
Closed
5 tasks

adopt connector best practices in google ads source #1568

sherifnada opened this issue Jan 7, 2021 · 1 comment · Fixed by #1726
Assignees
Milestone

Comments

@sherifnada
Copy link
Contributor

sherifnada commented Jan 7, 2021

Tell us about the problem you're trying to solve

We'd like to follow the published connector development best practices on this connector.

Since this connector is Singer based, for each of the below items please verify whether:

  • the singer tap already implements it
  • the singer tap does not implement it and would require forking the tap to implement
  • it's something we can implement easily on top of the Singer tap without forking it

If implementing any of the below items would require forking the Singer tap, please notify @sherifnada and move on to the next issue.

Checklist

  • Check connection verifies permissions upfront
  • Check connection failure emits actionable error messages. This will probably require you to think about the failure modes that can occur (e.g: timeout, permission not granted, token does not exist, etc.. -- make sure that you add custom integration tests to verify each of these failure modes, or unit tests if you can mock the HTTP requests involved)
  • If API connector: integration tests validate that every stream outputs data (you may need to seed the underlying API with fake data). This will establish a baseline confidence that the connector works correctly on every data stream. If this is not possible, please reach out to @sherifnada with the reason to discuss how to move forward.
  • If API connector: backs off on hitting rate limits (instead of just failing/exiting)
  • If API connector: implements incremental sync (where possible -- not all streams will allow this)
@sherifnada sherifnada added type/enhancement New feature or request area/connectors Connector related issues zazmic labels Jan 7, 2021
@vitaliizazmic
Copy link
Contributor

The estimate time to complete this task is 7h

Checklist

  • Check connection verifies permissions upfront - 1h
  • Check connection failure emits actionable error messages. -1h
  • If API connector: integration tests validate that every stream outputs data (you may need to seed the underlying API with fake data). - 2h
  • If API connector: backs off on hitting rate limits (instead of just failing/exiting) - 1h
  • If API connector: implements incremental sync (where possible -- not all streams will allow this) - 2h

vitaliizazmic added a commit that referenced this issue Jan 15, 2021
@sherifnada sherifnada added this to the Launch 1.0 milestone Jan 19, 2021
@sherifnada sherifnada linked a pull request Jan 23, 2021 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants