-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: code refactoring #15229
🎉Source Google Analytics Data Api: code refactoring #15229
Conversation
54bd978
to
befbc31
Compare
/test connector=connectors/source-google-analytics-data-api
Build FailedTest summary info:
|
/test connector=connectors/source-google-analytics-data-api
Build PassedTest summary info:
|
befbc31
to
cd52596
Compare
cd52596
to
c300efe
Compare
c300efe
to
26c5adb
Compare
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.
Tests + Oauth implementation looking great!
Some open questions...
docker_image := airbyte/$(notdir $(CURDIR)):dev | ||
|
||
run-build: | ||
docker build . -t ${docker_image} | ||
|
||
spec: | ||
@docker run --rm $(docker_image) spec | jq | ||
|
||
check: | ||
@docker run --rm -v $(PWD)/secrets:/secrets $(docker_image) check --config /secrets/config.json | jq | ||
|
||
discover: | ||
@docker run --rm -v $(PWD)/secrets:/secrets $(docker_image) discover --config /secrets/config.json | jq | ||
|
||
read: | ||
@docker run --rm -v $(PWD)/secrets:/secrets -v $(PWD)/integration_tests:/integration_tests $(docker_image) read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json | jq | ||
|
||
unittest-local: | ||
@python -m pytest unit_tests |
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.
what's this Makefile for?
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.
It is not necessary to have it in repository. I created it for running routine operations more quickly. It is only for development purpose and for convenience. For example check
is a long command but I am just typing make check
in my console
@@ -5,7 +5,7 @@ | |||
|
|||
from setuptools import find_packages, setup | |||
|
|||
MAIN_REQUIREMENTS = ["airbyte-cdk~=0.1", "google-analytics-data==0.11.2"] | |||
MAIN_REQUIREMENTS = ["airbyte-cdk~=0.1", "google-analytics-data==0.11.2", "PyJWT", "cryptography", "requests"] |
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.
lib versions should be pinned please
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
from source_google_analytics_data_api import utils | ||
|
||
|
||
class GoogleServiceKeyAuthenticator(requests.auth.AuthBase): |
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.
is this completely unique such that we can't use any of the cdk authenticator frameworks?
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 didn't find any proper base class in cdk
for service authentication. In some other connectors they are using OAuth class as a base class and re-implementing methods but it is basically the same as I do using requests.auth.AuthBase
. It is more clea[n/r] I think
import calendar | ||
import datetime | ||
|
||
|
||
def datetime_to_secs(dt: datetime.datetime) -> int: | ||
return calendar.timegm(dt.utctimetuple()) | ||
|
||
|
||
def string_to_date(d: str, f: str = "%Y-%m-%d") -> datetime.date: | ||
return datetime.datetime.strptime(d, f).date() | ||
|
||
|
||
def date_to_string(d: datetime.date, f: str = "%Y-%m-%d") -> str: | ||
return d.strftime(f) |
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.
nit: we generally use pendulum
library for datetime operations (it's basically datetime++)
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.
Thanks, I will keep it in mind. Should I rewrite these utils for using pendulum
or they are good as is for just doing formatting?
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.
this is fine if it works
/test connector=connectors/source-google-analytics-data-api
Build PassedTest summary info:
|
@roman-yermilov-gl |
0417db9
to
c861452
Compare
/test connector=connectors/source-google-analytics-data-api
Build PassedTest summary info:
|
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
c861452
to
4e5ddd2
Compare
/test connector=connectors/source-google-analytics-data-api
Build PassedTest summary info:
|
/publish connector=connectors/source-google-analytics-data-api
if you have connectors that successfully published but failed definition generation, follow step 4 here |
* Source: google analytics data api refactoring * google-analytics-data-api.md Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com> * updated spec: added custom reports field * doclint Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com> * auto-bump connector version [ci skip] Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com> Co-authored-by: Sergey Chvalyuk <grubberr@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
Refactoring of the Google analytics data API source. Use REST API calls instead of library and support a default set of reports
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.