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

Added new connector: source-google-analytics-data-api #1368

Closed
wants to merge 6 commits into from

Conversation

Luishfs
Copy link
Collaborator

@Luishfs Luishfs commented Mar 13, 2024

Description:

(Describe the high level scope of new or changed features)

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

@Luishfs Luishfs self-assigned this Mar 13, 2024
@Luishfs Luishfs requested review from williamhbaker and removed request for jgraettinger March 19, 2024 20:59
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
Removed unused files
Added new test files & modified test.flow.yaml
Fixed config.yaml
@Luishfs Luishfs linked an issue Mar 26, 2024 that may be closed by this pull request
@Luishfs Luishfs linked an issue Mar 26, 2024 that may be closed by this pull request
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.

See inline comments. Also we need to test the migration scenario, see here for more information on what this would entail.

@@ -4,6 +4,17 @@
"$schema": "https://json-schema.org/draft-07/schema#",
Copy link
Member

Choose a reason for hiding this comment

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

We should have capture snapshot output tests as well if at all possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@williamhbaker Does this account posses a valid dashboard so i can do that?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not honestly sure what a dashboard is in the context of google analytics. I think you set up the test property, though? So if its possible to add some data to 431462875 and capture it + snapshot, that would be ideal. Otherwise it'll be tough to feel confident the new version works, if we haven't been able to test it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is really hard as we need a real website with the hook inside the pages html.
We can try with other data sources, but cant put it inside the CI pipeline without a estuary version of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@williamhbaker How do you want to proceed? Test locally and validate the data?

Copy link
Member

Choose a reason for hiding this comment

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

My objective is to get confidence that the new connector outputs data in roughly the same way as the old connector. Since there aren't any major differences in the output schemas, that's one point of confidence.

I'm hearing that it is not practical to have automated CI output tests, and that's kind of a bummer but it is what it is.

It would certainly be nice to know that this new connector has some ability to capture data. If there's a way for you to test it locally and validate the data, that would be great, and you can just add a description of what you did here and we'll call it good based on that manual testing.

I'm imagining make a static HTML page and opening it in my browser locally that has the hook in it and using that to generate some data in google analytics. I don't know if that would actually work though. If it does, I'd wonder why we couldn't have that as a CI test, since I would think that once the data is in google analytics it would stay there for later test runs, but maybe that is a flawed assumption.

@Luishfs
Copy link
Collaborator Author

Luishfs commented Apr 9, 2024

@williamhbaker As discussed earlier, im closing this PR and using this PR as the main one for this import, now with the correct branch set inside estuary/connectors

@Luishfs Luishfs closed this Apr 9, 2024
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.

2 participants