Source Facebook Marketing (no-singer) #1443

eugene-kulak · 2020-12-24T11:07:42Z

Contributor Checklist

Thanks for contributing to Airbyte! Please complete the following items in order so we can review your PR.

Followed all the instructions in the locally generated checklist and your connector is functional & ready for review
Ran the standard test suite locally via ./gradlew :airbyte-integrations:connectors:source-<your_source_name>:standardSourceTestPython and pasted the summarized output as a comment in this PR

Reviewer Pre-merge Checklist

Finished iterating with the PR author on the code*
Created a branch off master to merge this PR into*
Inject the credentials in CI via ./tools/integrations/ci_credentials.sh and .github/workflows/test-command.yml*
Added the credentials for this integration to Github secrets
Run standard tests on this branch by commenting /test connector=<name>*
Add entry in airbyte-config/init/src/main/resources/seed/source_definitions.yaml to use the new source in Airbyte core
Deployed the connector to Dockerhub via ./tools/integrations/manage.sh publish airbyte-integrations/connectors/source-<name>

Documentation

Add docs in docs/integrations/sources/ folder in line with the documentation template found in docs/contributing-to-airbyte/templates/integration-documentation-template.md.
Add link to create docs file to docs/SUMMARY.md
Include a link to the documentation in the README.md

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/common.py

arhip11 · 2020-12-24T18:35:26Z

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/client.py

+
+    @staticmethod
+    def _find_account(account_id: str):
+        accounts = fb_user.User(fbid='me').get_ad_accounts()


What do you think about next to emulate find if?

could you please give me some example?
I think the current version is not too difficult to read, but maybe I'm wrong...

The current code is OK to read, agree. I was thinking about:
return next((x for x in accounts if x["account_id"] == account_id),)

well, I like this magic, but not in the code :)

arhip11 · 2020-12-26T18:13:40Z

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/client.py

+
+    def read_stream(self, stream: AirbyteStream) -> Generator[AirbyteRecordMessage, None, None]:
+        """Yield records from stream"""
+        method = self._stream_methods.get(stream.name)


I would consider moving this to the base class.
method = self.get_method(stream.name) would be more concise and expressive.

moved to base class, and fixed existing derived classes to accept kwargs

arhip11 · 2020-12-26T18:16:12Z

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/client.py

+        print(stream.json_schema)
+        return []
+
+    def read_stream(self, stream: AirbyteStream) -> Generator[AirbyteRecordMessage, None, None]:


Why do you override base class method? Looks like the implementation is identical.

to pass fields, please see my updates

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/client.py

eugene-kulak · 2020-12-29T10:59:23Z

Test run finished after 36085 ms
[         2 containers found      ]
[         0 containers skipped    ]
[         2 containers started    ]
[         0 containers aborted    ]
[         2 containers successful ]
[         0 containers failed     ]
[         7 tests found           ]
[         0 tests skipped         ]
[         7 tests started         ]
[         0 tests aborted         ]
[         7 tests successful      ]
[         0 tests failed          ]

…keting

jrhizor

This generally looks good. Just a few minor comments for now.

I'm going to run some tests on this later tonight or tomorrow morning.

airbyte-integrations/connectors/source-facebook-marketing/build.gradle

jrhizor · 2020-12-29T23:14:22Z

airbyte-config/init/src/main/resources/seed/source_definitions.yaml

+  name: Facebook Marketing
+  dockerRepository: airbyte/source-facebook-marketing
+  dockerImageTag: 0.1.0
+  documentationUrl: https://hub.docker.com/r/airbyte/source-facebook-marketing
 - sourceDefinitionId: 74d47f79-8d01-44ac-9755-f5eb0d7caacb
  name: Facebook Marketing APIs


In a separate PR we should delete the Singer one.

docs/integrations/sources/facebook-marketing.md

jrhizor · 2020-12-30T00:45:15Z

airbyte-integrations/connectors/source-freshdesk/.dockerignore

@@ -0,0 +1,7 @@
+*


Should this file be in this PR?

airbyte-integrations/connectors/source-facebook-marketing/unit_tests/test_client.py

jrhizor · 2020-12-30T00:52:17Z

...integrations/connectors/source-facebook-marketing/source_facebook_marketing/client/client.py

+
+        fields = list(set(fields) - set(self.INVALID_INSIGHT_FIELDS))
+
+        # Some automatic fields (primary-keys) cannot be used as 'fields' query params.


This comment should be moved to where INVALID_INSIGHT_FIELDS is defined.

…d.gradle Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

…_tests/test_client.py Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

jrhizor

The integration tests are working and everything looks good, but there are errors when I run the unit tests with ./gradlew :airbyte-integrations:connectors:source-facebook-marketing:unitTest from the airbyte root directory or .venv/bin/python -m pytest unit_tests from the source's directory.

Messages must be strings for AirbyteLogMessage.

This is the only issue blocking this PR.

eugene-kulak · 2020-12-31T00:21:11Z

@jrhizor thanks, I have fixed logger usage in the last commit

jrhizor · 2020-12-31T01:30:50Z

You'll also have to run ./gradlew format to format the JSON files added in the PR.

jrhizor · 2021-01-05T19:00:29Z

I'm running the build again from #1525

Please look into build failures if there are any. If it succeeds I'll merge this.

Sorry for the delay.

jrhizor · 2021-01-05T20:57:59Z

It looks like the changes here introduced a dependency issue as well.

It's really slow for me to create a separate PR and wait for builds to iterate with feedback. Can you reopen this PR on an origin branch instead of a fork so the builds can run automatically? Then you can iterate until the build is passing without any involvement from me, and it will be obvious when this is ready to merge.

Here's a failing build run: https://github.com/airbytehq/airbyte/runs/1651989807

Alternatively, you could ping me on this PR once ./gradlew build is passing locally.

eugene-kulak · 2021-01-11T16:25:06Z

closed in favor of #1552

eugene-kulak added 4 commits December 24, 2020 13:05

source Facebook Marketing (no-singer)

dc5f72e

update sample config

9cef6e3

fix source import

bd81d25

add fixme

17884b2

arhip11 reviewed Dec 26, 2020

View reviewed changes

eugene-kulak added 9 commits December 29, 2020 01:52

fix tests, account fetch and stream reading

0f7e9c9

implement schema resolver

6343bfc

add CI creds

b59d50d

register as standard source

a422faf

fix tests

7a21b16

format

0b5d914

fix Dockerfile.test

1ff5878

fix python3.7 compatibility

c909a4b

clean up

555038e

eugene-kulak added 5 commits December 29, 2020 13:05

address comments from @arhip11

5530fa7

address comments from @arhip11

a842673

Merge remote-tracking branch 'origin/master' into source-facebook-mar…

ced47e8

…keting

clean up

e285d71

"fix" logger usage, better fix possible in airbytehq#1279

c99e121

eugene-kulak changed the title ~~WIP: Source Facebook Marketing (no-singer)~~ Source Facebook Marketing (no-singer) Dec 29, 2020

eugene-kulak added 2 commits December 29, 2020 18:55

fix incorrect logger usage

3731637

fix incorrect logger usage (2)

14c9086

This was referenced Dec 29, 2020

Source Facebook Marketing: Add option to include deleted records #1477

Closed

Source Facebook Marketing: Add incremental update #1478

Closed

jrhizor reviewed Dec 30, 2020

View reviewed changes

eugene-kulak and others added 4 commits December 30, 2020 05:47

Update airbyte-integrations/connectors/source-facebook-marketing/buil…

19b2de3

…d.gradle Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

Update docs/integrations/sources/facebook-marketing.md

929d2ac

Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

Update airbyte-integrations/connectors/source-facebook-marketing/unit…

79a33f3

…_tests/test_client.py Co-authored-by: Jared Rhizor <me@jaredrhizor.com>

address CR changes

0d1017b

drop accidentally added .dockerinore file

54b1953

jrhizor suggested changes Dec 30, 2020

View reviewed changes

fix logger usage (3)

dee3d88

format

13c2eee

eugene-kulak closed this Jan 11, 2021

snyk-bot mentioned this pull request Aug 23, 2021

[Snyk] Upgrade dayjs from 1.8.35 to 1.10.6 schlattk/airbyte#2

Open

karinakuz added connectors/sources-api connectors/source/facebook-marketing labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Facebook Marketing (no-singer) #1443

Source Facebook Marketing (no-singer) #1443

eugene-kulak commented Dec 24, 2020 •

edited

arhip11 Dec 24, 2020

eugene-kulak Dec 29, 2020 •

edited

arhip11 Dec 29, 2020

eugene-kulak Dec 29, 2020

arhip11 Dec 26, 2020

eugene-kulak Dec 29, 2020

arhip11 Dec 26, 2020

eugene-kulak Dec 29, 2020

eugene-kulak commented Dec 29, 2020

jrhizor left a comment

jrhizor Dec 29, 2020

jrhizor Dec 30, 2020

eugene-kulak Dec 30, 2020

jrhizor Dec 30, 2020

jrhizor left a comment

eugene-kulak commented Dec 31, 2020

jrhizor commented Dec 31, 2020

jrhizor commented Jan 5, 2021

jrhizor commented Jan 5, 2021 •

edited

eugene-kulak commented Jan 11, 2021


		fields = list(set(fields) - set(self.INVALID_INSIGHT_FIELDS))

		# Some automatic fields (primary-keys) cannot be used as 'fields' query params.

Source Facebook Marketing (no-singer) #1443

Source Facebook Marketing (no-singer) #1443

Conversation

eugene-kulak commented Dec 24, 2020 • edited

Contributor Checklist

Reviewer Pre-merge Checklist

Documentation

Choose a reason for hiding this comment

eugene-kulak Dec 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-kulak commented Dec 29, 2020

jrhizor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhizor left a comment

Choose a reason for hiding this comment

eugene-kulak commented Dec 31, 2020

jrhizor commented Dec 31, 2020

jrhizor commented Jan 5, 2021

jrhizor commented Jan 5, 2021 • edited

eugene-kulak commented Jan 11, 2021

eugene-kulak commented Dec 24, 2020 •

edited

eugene-kulak Dec 29, 2020 •

edited

jrhizor commented Jan 5, 2021 •

edited