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.

Sign up for GitHub

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

Jump to bottom

🎉 fix Google Adwords CI & publish new version #4205

Merged
merged 10 commits into from
Jun 25, 2021
Merged

🎉 fix Google Adwords CI & publish new version #4205

merged 10 commits into from
Jun 25, 2021

Conversation

po3na4skld
Copy link
Contributor

@po3na4skld po3na4skld commented Jun 18, 2021 •
edited
Loading

What

It solves #2929

How

Changing the states to SAT

Recommended reading order

  1. configured_catalog.json

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Unit & integration tests added as appropriate (and are passing)
  • /test connector=connectors/<name> command as documented here is passing.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Associated tickets have been closed & stakeholders notified

@po3na4skld po3na4skld requested review from keu and vitaliizazmic June 18, 2021 14:50
@github-actions github-actions bot added the area/connectors Connector related issues label Jun 18, 2021
@po3na4skld po3na4skld linked an issue Jun 18, 2021 that may be closed by this pull request
fix Google Adwords CI & publish new version #2929
Closed
@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 24, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-adwords-singer

🕑 connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/967689787
✅ connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/967689787

@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 24, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-adwords-singer

🕑 connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/968476369
✅ connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/968476369

vitaliizazmic
vitaliizazmic reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@vitaliizazmic vitaliizazmic left a comment

Choose a reason for hiding this comment

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

@po3na4skld as I see, you have only enabled SAT for the Source. Could you please clarify issue and how it helped fix bug? Also could you add CHANGELOG.md and don't forget to bump version and publish.

@keu
Copy link
Contributor

keu commented Jun 25, 2021 •
edited
Loading

@po3na4skld @vitaliizazmic CHANGELOG.md has being moved to <connector>.md file

keu
keu approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@keu keu left a comment

Choose a reason for hiding this comment

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

lgtm, just small fixes

@keu keu requested review from davinchia, sherifnada and tuliren June 25, 2021 09:40
@po3na4skld
Copy link
Contributor Author

@po3na4skld as I see, you have only enabled SAT for the Source. Could you please clarify issue and how it helped fix bug? Also could you add CHANGELOG.md and don't forget to bump version and publish.

@vitaliizazmic I added them because previous tests were failing due to error: Expected the first incremental sync to produce records. As we have all streams with full refresh, we decided to remove incremental testings as it is not supported for this singer for now.

@po3na4skld po3na4skld requested a review from vitaliizazmic June 25, 2021 10:03
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 25, 2021
@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 25, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-google-adwords-singer

🕑 connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/971136758
✅ connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/971136758

@po3na4skld
Copy link
Contributor Author

po3na4skld commented Jun 25, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-google-adwords-singer

🕑 connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/972349297
✅ connectors/source-google-adwords-singer https://github.com/airbytehq/airbyte/actions/runs/972349297

sherifnada
sherifnada approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

@po3na4skld can you update the PR description to explain what changes this PR made? Imagine someone reading the commit log -- "fix the connector" doesn't tell me what this PR is doing 😜

docs/integrations/sources/google-adwords.md

| Version | Date | Pull Request | Subject |
| :------ | :-------- | :----- | :------ |
| 0.1.2 | 2021-06-25 | [4205](https://github.com/airbytehq/airbyte/pull/4205) | Set up CDK SAT tests. Incremental tests are disabled due to unsupported state structure in current tests: required structure: {stream_name: cursor_value} given {‘bookmarks’: {stream_name: cursor_value}} |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| 0.1.2 | 2021-06-25 | [4205](https://github.com/airbytehq/airbyte/pull/4205) | Set up CDK SAT tests. Incremental tests are disabled due to unsupported state structure in current tests: required structure: {stream_name: cursor_value} given {‘bookmarks’: {stream_name: cursor_value}} |
| 0.1.2 | 2021-06-25 | [4205](https://github.com/airbytehq/airbyte/pull/4205) | Set up CDK SAT tests. |

Copy link
Contributor

Choose a reason for hiding this comment

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

@po3na4skld the log message is wrong, you bump version to 0.2.5, but logs say about 0.1.2.
Also why did you bumped 0.2.3 to 0.2.5, where is 0.2.4?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch eguene.

skipping the version isn't a big problem, but we should definitely list the correct version in the changelog

@po3na4skld po3na4skld merged commit 40aed22 into master Jun 25, 2021
@po3na4skld po3na4skld deleted the po3na4skld/fix-google-adwords-ci branch June 25, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

@keu keu keu approved these changes

@vitaliizazmic vitaliizazmic vitaliizazmic approved these changes

@tuliren tuliren tuliren approved these changes

@davinchia davinchia Awaiting requested review from davinchia

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix Google Adwords CI & publish new version
5 participants
@po3na4skld @keu @tuliren @sherifnada @vitaliizazmic