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

Adding source-google-analytics-data-api #1462

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Luishfs
Copy link
Collaborator

@Luishfs Luishfs commented Apr 9, 2024

Description:

Importing source-google-analytics-data-api and making necessary changes to allow for backward compatibility with our current docker image

Notes for reviewers:
This PR was created to substitute an older PR which was created by my fork from estuary/connectors

Discover tests

Jsondiff link with discover tests. Left json is the initial snapshot, right json is the imported connector snapshot

Spec tests

jsondiff link with spec tests. Left json is the initial snapshot. Right json is the imported connector snapshot

Capture tests

Capture tests were executed on a created Google Analytics account, and was populated using a localhost HTML static page with google analytics hooks linked in the HTML header.

This Source capture test can vary daily since its based on access metrics. Because of that, capture snapshot was removed.

JSON data example

In order to compare captures in the future, one can use the following JSON object:
snapshots__capture__capture.stdout.json

End-to-End tests

Tests were made using a local flow setup and a free-tier supabase db.

Connector Shard/Spec

No issues were found while starting the connector
Screenshot from 2024-04-16 13-11-57

OAuth2

No issues were found when running OAuth2
OAuth connection:
Screenshot from 2024-04-16 10-45-52

Generated Tables

All 8 discovered tables are being generated
Screenshot from 2024-04-16 13-09-54

Data Example

Materialization example:
Screenshot from 2024-04-16 13-10-09


This change is Reviewable

copying the image behaviour
Remote Repo URL: [email protected]:airbytehq/airbyte.git
Source name: 019153f
Source Commit ID: 019153f178d221bbf602c21efea54f78042544ca
Source Repo Prefix: airbyte-integrations/connectors/source-google-analytics-data-api/
Import Path: source-google-analytics-data-api/
License Type: MIT
License Path: airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml

git-merge-subpath: 019153f178d221bbf602c21efea54f78042544ca airbyte-integrations/connectors/source-google-analytics-data-api source-google-analytics-data-api
@williamhbaker
Copy link
Member

The tests are broken; let me know when those are passing.

@Luishfs Luishfs force-pushed the luis/source-google-analytics-data-api branch 2 times, most recently from f20dd7e to 7d2ded0 Compare April 9, 2024 13:45
@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 9, 2024

@williamhbaker tests are now passing, deployment is breaking CI pipeline

@williamhbaker
Copy link
Member

Can you provide a diff that compares the existing connector's outputs for discovery and spec with the final version of the one from this PR? The changes are spread between two commits here so I can't clearly tell what is changing.

@Luishfs Luishfs force-pushed the luis/source-google-analytics-data-api branch from d8aa0d6 to 76df21a Compare April 10, 2024 19:15
Modified spec.json to match fixups
@Luishfs Luishfs force-pushed the luis/source-google-analytics-data-api branch 4 times, most recently from f70f4ba to 0d92ba8 Compare April 11, 2024 18:38
@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 11, 2024

@williamhbaker Added jsondiff and capture snapshot explanation in this PR description, along with some small fixes that were occuring with a warning log on api-quota method

@williamhbaker
Copy link
Member

The discovery diff looks good, thanks for that.

There are some obvious issues with the new spec output. The documentation link is wrong, and there are missing types for some of the fields, to name a couple. Please review the spec output and make corrections so that the new connector is compatible with the old connector.

Updated python requirements to allow for CI pipeline build
@Luishfs Luishfs force-pushed the luis/source-google-analytics-data-api branch from 0d92ba8 to c56a832 Compare April 12, 2024 01:21
@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 12, 2024

@williamhbaker Spec diff has been updated, Jsondiff link

Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 16, 2024

@williamhbaker Since now i am able to setup locally Flow Local, i wanted to run End-to-End tests on all the import PRs just to be sure. Thats why i've asked again for your review on this PR.
Will update the PR description with the end-to-end tests and ping you

@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 16, 2024

@williamhbaker Just updated the PR description
What do you think of how the examples are being displayed?

@williamhbaker
Copy link
Member

@williamhbaker Just updated the PR description What do you think of how the examples are being displayed?

Looks great! Might be a bit overkill TBH 😄. As long as you've tested it end-to-end and documented that (a simple "manually tested by running end to end on a local stack" would be sufficient), or not tested it and documented why, that's enough - don't necessarily need the photographic proof if it saves you some time.

@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 16, 2024

@williamhbaker Got it! im going to update it in a more simple term on source-twilio and source-salesforce

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

Successfully merging this pull request may close these issues.

source-google-analytics-data-api: import to Estuary python CDK
2 participants