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

Source GoogleAds: add end_date to config #8669

Merged
merged 19 commits into from
Jan 25, 2022
Merged

Source GoogleAds: add end_date to config #8669

merged 19 commits into from
Jan 25, 2022

Conversation

iberchid
Copy link
Contributor

@iberchid iberchid commented Dec 9, 2021

What

Relates to issue #8666

How

Added end_date to spec and updated the source.py and streams.py files to allow for setting an end_date.

Recommended reading order

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

🚨 User Impact 🚨

No changes, because when the parameter is not provided the default value is used which is identical to the previous implementation.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • 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
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • 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
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the new connector version is published, connector version bumped in the seed directory 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

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Dec 9, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Dec 9, 2021
Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

See comments.

@marcosmarxm
Copy link
Member

@iberchid please run unit tests to validate your change also please add a new unit test with a specific end_date to validate your changes.

@marcosmarxm
Copy link
Member

@sherifnada can you review the UX modification this PR makes?

@marcosmarxm marcosmarxm self-assigned this Dec 10, 2021
@iberchid
Copy link
Contributor Author

Hi @marcosmarxm , Thanks for your comments, I updated the scripts according to your recommendations. I also ran existing unit tests and added a few more:

image

@marcosmarxm
Copy link
Member

@iberchid can you solve the conflicts and make sure the ./gradlew airbyte-integrations:connectors:source-google-ads:integrationTest is working?

@marcosmarxm
Copy link
Member

marcosmarxm commented Jan 3, 2022

@iberchid can you solve the conflicts? and share the output from the command above

@iberchid
Copy link
Contributor Author

iberchid commented Jan 10, 2022

Hello @marcosmarxm , I resolved the conflicts and reran the unit tests both in .venv and gradle, below is the output.

1-Locally in .venv : The test test_streams_count failed in test_source.py , which expected result was changed here. The remaining tests passed.

image

2- Gradle: The failures are related to secrets/config.json, these tests, added in this commit, use the config.json file, which cannot be accessed with Gradle. The remaining tests passed.

image

image

@marcosmarxm
Copy link
Member

You need to create a folder called secrets and add a file inside with the config to run integration tests and check connection.

@iberchid
Copy link
Contributor Author

Here it is @marcosmarxm :

image

@iberchid
Copy link
Contributor Author

Hi @marcosmarxm , what is the status of this PR? Are there any other TODOs left for me?

@marcosmarxm marcosmarxm temporarily deployed to more-secrets January 25, 2022 01:44 Inactive
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jan 25, 2022
@marcosmarxm marcosmarxm temporarily deployed to more-secrets January 25, 2022 01:47 Inactive
@octavia-squidington-iii octavia-squidington-iii temporarily deployed to more-secrets January 25, 2022 01:50 Inactive
@marcosmarxm marcosmarxm temporarily deployed to more-secrets January 25, 2022 02:52 Inactive
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants