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

New Source: Display&Video 360 #11828

Merged
merged 25 commits into from
Sep 28, 2022

Conversation

iberchid
Copy link
Contributor

@iberchid iberchid commented Apr 8, 2022

What

Relates to #10850
Closed #2984

How

A connector integration for a new source (DV360).

Recommended reading order

  1. spec.py
  2. source.py
  3. streams.py

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Tests

Unit

image

Integration

image

Acceptance

Put your acceptance tests output here.

image

┆Issue is synchronized with this Monday item by Unito

@github-actions github-actions bot added the area/connectors Connector related issues label Apr 8, 2022
@iberchid iberchid changed the title New Source: Display&Video 360 Draft: New Source: Display&Video 360 Apr 8, 2022
@marcosmarxm
Copy link
Member

Thanks for creating this connector @iberchid did you have the chance to run the integration test?

@iberchid iberchid force-pushed the iberchid/new_connector_dv360 branch from 2255499 to ca2dfe6 Compare April 14, 2022 15:17
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Apr 14, 2022
@iberchid
Copy link
Contributor Author

iberchid commented Apr 14, 2022

Hi @marcosmarxm , I added the results of the integrations tests to the description.
I am not successful with the acceptance tests however, I am always getting the error that the docker image is not found, even found even though I run docker build . beforehand.

image

The docker image gets built without name:

image

Do you have any suggestions?

@marcosmarxm
Copy link
Member

What command are you using to build the image? You can run ./gradlew airbyte-integrations:connectors:source-name:integrationTest from the root folder too.

@marcosmarxm marcosmarxm self-assigned this Apr 20, 2022
@iberchid
Copy link
Contributor Author

iberchid commented Apr 21, 2022

Hi @marcosmarxm

The docker image was not tagged with the connector's name, so I did it manually and it worked. I am using docker build .

I still have 4 failing acceptance tests, could you give me some guidance here ?

image

Here is the output of my check method, I do not understand why the check test fails

image

Here are more details about the errors:

image

image

image

image

@CLAassistant
Copy link

CLAassistant commented May 5, 2022

CLA assistant check
All committers have signed the CLA.

@marcosmarxm marcosmarxm added the waiting-for-cred Waiting for CI credentials label May 9, 2022
@iberchid iberchid changed the title Draft: New Source: Display&Video 360 New Source: Display&Video 360 May 16, 2022
@marcosmarxm
Copy link
Member

Sorry the long delay in reply here @iberchid did you solve the problem with acceptance tests? do you need any help?

@iberchid
Copy link
Contributor Author

Hello @marcosmarxm , not yet, I still have these 4 failing tests.

image

@iberchid
Copy link
Contributor Author

Hi @marcosmarxm , any hints?

@marcosmarxm
Copy link
Member

sorry @iberchid can you send me dm in slack so I can heplp you?

@iberchid
Copy link
Contributor Author

Thanks, I wrote you on slack @marcosmarxm

@marcosmarxm
Copy link
Member

@iberchid let me know when you submit the new code with the changes!

@alafanechere alafanechere added bounty bounty-XL Maintainer program: claimable extra large bounty PR labels Jun 2, 2022
@marcosmarxm marcosmarxm removed the bounty-XL Maintainer program: claimable extra large bounty PR label Jun 2, 2022
@marcosmarxm marcosmarxm self-assigned this Jun 2, 2022
@marcosmarxm
Copy link
Member

@iberchid please share unit and integration tests

@iberchid
Copy link
Contributor Author

iberchid commented Jun 2, 2022

Hi @marcosmarxm below are the results:

unit tests:
image

acceptance tests
image

I still have 2 tests still failing, attached is the detailed output:
failed_tests.txt

Do these tests only take the name of the stream and try to read all the available fields in it? Because DV360 in a bit special, not all fields within the same stream can be fetched at the same time, many are not compatible with each other.

@marcosmarxm
Copy link
Member

1st error: there is a stream the connector is not getting any record
2nd error: looks for a incremental stream the connector is not producing a state

Comment on lines 26 to 32
def chunk_date_range(
start_date: str,
field: str,
end_date: str = None,
range_days: int = None,
) -> Iterable[Mapping[str, any]]:
"""
Copy link
Member

Choose a reason for hiding this comment

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

please run ./gradlew format and commit only your files this is not the default formating style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still facing some issues with blackFormat. I have python v3.8 and installed sudo pip install --upgrade click==8.0.1

Attached are the errors I am getting, do you have any hints for me? Thanks.

err_format_gradle.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcosmarxm any hints?

@marcosmarxm
Copy link
Member

marcosmarxm commented Sep 27, 2022

/publish connector=connectors/source-dv-360 run-tests=false

🕑 Publishing the following connectors:
connectors/source-dv-360
https://github.com/airbytehq/airbyte/actions/runs/3137986538


Connector Did it publish? Were definitions generated?
connectors/source-dv-360

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@marcosmarxm marcosmarxm merged commit 2aea783 into airbytehq:master Sep 28, 2022
@marcosmarxm
Copy link
Member

Sorry the long delay to merge this @iberchid thanks for the contribution

robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* Add dv360 connector source

* update query methods

* sanitize fields and fetch only fields in config_catalog

* Read and check methods + rmv extra spaces from schema fields

* add timezone in query + log error message in Read method

* start incr streamand tests

* Add unit tests and documentation

* rmv chunck date method

* Add incremental stream

* Add dv360 connector

* Add dv360 connector

* rmv .hbs from unit test file

* Add BOOTSTRAP.md file

* Delete airbyte-integrations/connectors/source-dv360 directory

Delete old folder

* Add Docs

* update config_Catalog

* filter last row in case of an additional empty row for the sum of the metrics + add required fields in unique_reach_audience stream

* Update read methode by removing summary row in the case of the standard report

* Update invalid_config and spec files

* Add first state msg in Read method

* Code format

* format connector

* add to seed file

* auto-bump connector version [ci skip]

Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* Add dv360 connector source

* update query methods

* sanitize fields and fetch only fields in config_catalog

* Read and check methods + rmv extra spaces from schema fields

* add timezone in query + log error message in Read method

* start incr streamand tests

* Add unit tests and documentation

* rmv chunck date method

* Add incremental stream

* Add dv360 connector

* Add dv360 connector

* rmv .hbs from unit test file

* Add BOOTSTRAP.md file

* Delete airbyte-integrations/connectors/source-dv360 directory

Delete old folder

* Add Docs

* update config_Catalog

* filter last row in case of an additional empty row for the sum of the metrics + add required fields in unique_reach_audience stream

* Update read methode by removing summary row in the case of the standard report

* Update invalid_config and spec files

* Add first state msg in Read method

* Code format

* format connector

* add to seed file

* auto-bump connector version [ci skip]

Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/dv-360 internal waiting-for-cred Waiting for CI credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New source: Google Display & Video 360
7 participants