-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Source Google Analytics Data API: ability to add multiple property ids #30152
Source Google Analytics Data API: ability to add multiple property ids #30152
Conversation
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
1b198b1
to
e0e2003
Compare
7858762
to
cc5db93
Compare
582e75a
to
1975405
Compare
|
Step | Result |
---|---|
Connector package install | ✅ |
Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
@@ -43,7 +43,7 @@ def patch_base_class(): | |||
@pytest.fixture | |||
def config(): | |||
return { | |||
"property_id": "108176369", | |||
"property_ids": ["108176369"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add unit test with old config and try test backward compatibility in that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in scope of migration
@@ -4,7 +4,7 @@ | |||
"$schema": "https://json-schema.org/draft-07/schema#", | |||
"title": "Google Analytics (Data API) Spec", | |||
"type": "object", | |||
"required": ["property_id", "date_ranges_start_date"], | |||
"required": ["property_ids", "date_ranges_start_date"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you test those changes manually for backward compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, but it seems I missed something. Now migration is prepared and everithing is tested
|
||
for report in _config["custom_reports"]: | ||
invalid_dimensions = set(report["dimensions"]) - dimensions | ||
if invalid_dimensions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add spaces between condition blocks and add comments for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
raise wrong_property_id_error | ||
|
||
if not metadata: | ||
return False, "failed to get metadata, over quota, try later" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message better start with upper case letter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -39,6 +39,7 @@ def patch_base_class(mocker): | |||
|
|||
return { | |||
"config": { | |||
"property_ids": ["496180525"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an old config has not property_ids
what happens next?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured out in scope of migration
|
Step | Result |
---|---|
Connector package install | ✅ |
Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
9e0dc49
to
a3a4f1f
Compare
4bfe2e9
to
815f1ed
Compare
|
Step | Result |
---|---|
Connector package install | ✅ |
Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ❌ |
Code format checks | ✅ |
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
|
Step | Result |
---|---|
Connector package install | ✅ |
Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
Unit tests | ✅ |
Acceptance tests | ✅ |
Code format checks | ✅ |
Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
Connector version semver check | ✅ |
Connector version increment check | ✅ |
QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run tests with old format config
The tests were run with old config. If format is new, it means migration was applied during CI run |
What
#24739
How
Property ID is now a list. Set of streams is generated based on each property id