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

SATS: fix backwards compatibility #16429

Merged
merged 7 commits into from
Sep 12, 2022
Merged

SATS: fix backwards compatibility #16429

merged 7 commits into from
Sep 12, 2022

Conversation

davydov-d
Copy link
Collaborator

What

SATs declare widening of a data type is ok but in reality tests do not pass in a 'integer'-> '[integer, string]' type change. Fixing it here

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-notion
  • source-pardot
  • source-amplitude
  • source-mailchimp
  • source-onesignal
  • source-freshsales
  • source-commercetools
  • source-okta
  • source-prestashop
  • source-outreach
  • source-pipedrive
  • source-facebook-marketing
  • source-cart
  • source-greenhouse
  • source-klaviyo
  • source-youtube-analytics
  • source-lever-hiring
  • source-salesloft
  • source-twilio
  • source-asana
  • source-openweather
  • source-azure-table
  • source-sentry
  • source-facebook-pages
  • source-tplcentral
  • source-freshcaller
  • source-retently
  • source-plaid
  • source-zendesk-talk
  • source-airtable
  • source-appsflyer
  • source-chargebee
  • source-linnworks
  • source-amazon-seller-partner
  • source-instagram
  • source-surveymonkey
  • source-gitlab
  • source-braintree
  • source-zenloop
  • source-posthog
  • source-freshservice
  • source-recharge
  • source-paystack
  • source-salesforce
  • source-monday
  • source-amazon-sqs
  • source-strava
  • source-quickbooks-singer
  • source-zendesk-sunshine
  • source-harvest
  • source-google-search-console
  • source-google-ads
  • source-amazon-ads
  • source-sendgrid
  • source-drift
  • source-delighted
  • source-iterable
  • source-mailgun
  • source-pinterest
  • source-lemlist
  • source-github
  • source-confluence

@davydov-d
Copy link
Collaborator Author

davydov-d commented Sep 8, 2022 •
edited by github-actions bot

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3013624629
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3013624629
No Python unittests run

Build Passed

Test summary info:

All Passed

@davydov-d davydov-d mentioned this pull request Sep 8, 2022
Merged
@lazebnyi lazebnyi self-requested a review September 8, 2022 13:10
sherifnada
sherifnada suggested changes Sep 8, 2022
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.

@davydov-d could you add a unit test to verify this behavior?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-youtube-analytics
  • source-salesloft
  • source-amazon-seller-partner
  • source-delighted
  • source-zenloop
  • source-onesignal
  • source-freshsales
  • source-amazon-sqs
  • source-prestashop
  • source-mailgun
  • source-amplitude
  • source-confluence
  • source-tplcentral
  • source-zendesk-talk
  • source-appsflyer
  • source-recharge
  • source-airtable
  • source-facebook-pages
  • source-lemlist
  • source-harvest
  • source-lever-hiring
  • source-twilio
  • source-chargebee
  • source-greenhouse
  • source-quickbooks-singer
  • source-freshcaller
  • source-azure-table
  • source-asana
  • source-retently
  • source-surveymonkey
  • source-braintree
  • source-iterable
  • source-gitlab
  • source-sentry
  • source-instagram
  • source-plaid
  • source-paystack
  • source-mailchimp
  • source-salesforce
  • source-drift
  • source-openweather
  • source-pardot
  • source-zendesk-sunshine
  • source-amazon-ads
  • source-google-ads
  • source-github
  • source-outreach
  • source-klaviyo
  • source-commercetools
  • source-okta
  • source-google-search-console
  • source-posthog
  • source-strava
  • source-pinterest
  • source-notion
  • source-pipedrive
  • source-freshservice
  • source-cart
  • source-monday
  • source-linnworks
  • source-facebook-marketing
  • source-sendgrid

@davydov-d
Copy link
Collaborator Author

davydov-d commented Sep 9, 2022 •
edited by github-actions bot

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3020952214
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/3020952214
No Python unittests run

Build Passed

Test summary info:

All Passed

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-gitlab
  • source-openweather
  • source-zendesk-talk
  • source-linnworks
  • source-amplitude
  • source-cart
  • source-google-search-console
  • source-pipedrive
  • source-github
  • source-twilio
  • source-facebook-pages
  • source-pardot
  • source-outreach
  • source-pinterest
  • source-tplcentral
  • source-delighted
  • source-lever-hiring
  • source-zenloop
  • source-mailgun
  • source-appsflyer
  • source-okta
  • source-sentry
  • source-chargebee
  • source-surveymonkey
  • source-google-ads
  • source-strava
  • source-freshservice
  • source-commercetools
  • source-asana
  • source-harvest
  • source-airtable
  • source-salesforce
  • source-instagram
  • source-facebook-marketing
  • source-salesloft
  • source-onesignal
  • source-paystack
  • source-braintree
  • source-amazon-sqs
  • source-confluence
  • source-prestashop
  • source-sendgrid
  • source-quickbooks-singer
  • source-klaviyo
  • source-plaid
  • source-greenhouse
  • source-notion
  • source-retently
  • source-zendesk-sunshine
  • source-freshcaller
  • source-lemlist
  • source-drift
  • source-mailchimp
  • source-freshsales
  • source-amazon-ads
  • source-iterable
  • source-azure-table
  • source-posthog
  • source-monday
  • source-youtube-analytics
  • source-recharge
  • source-amazon-seller-partner

@davydov-d
Copy link
Collaborator Author

@davydov-d could you add a unit test to verify this behavior?

@sherifnada sure, done. I've also separated spec validation from catalog validation because I believe the behavior should be different in these cases

alafanechere
alafanechere requested changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

@davydov-d could you please explain your motivation for this change? Type widening sounds dangerous at first sight. Is it your approach to changing a spec type but still supporting the previous type?

@davydov-d
Copy link
Collaborator Author

@davydov-d could you please explain your motivation for this change? Type widening sounds dangerous at first sight. Is it your approach to changing a spec type but still supporting the previous type?

@alafanechere Yes. here I need to change data type from integer to string but support legacy integer values as well. That's why here I'm letting <old_type> -> [<old_type>, <new_type>] change to be valid (only for spec, not for catalog). Besides, it is stated to be a valid use case here, but is not at the moment

alafanechere
alafanechere approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Thank you for the clarification. LGTM

airbyte-integrations/bases/source-acceptance-test/CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-cart
  • source-strava
  • source-delighted
  • source-posthog
  • source-iterable
  • source-facebook-pages
  • source-okta
  • source-amplitude
  • source-prestashop
  • source-paystack
  • source-linnworks
  • source-google-ads
  • source-chargebee
  • source-lever-hiring
  • source-salesforce
  • source-greenhouse
  • source-freshsales
  • source-harvest
  • source-zenloop
  • source-drift
  • source-youtube-analytics
  • source-pipedrive
  • source-outreach
  • source-instagram
  • source-freshservice
  • source-surveymonkey
  • source-confluence
  • source-pinterest
  • source-lemlist
  • source-google-search-console
  • source-sentry
  • source-salesloft
  • source-commercetools
  • source-zendesk-sunshine
  • source-openweather
  • source-mailgun
  • source-mailchimp
  • source-retently
  • source-sendgrid
  • source-azure-table
  • source-klaviyo
  • source-amazon-ads
  • source-plaid
  • source-pardot
  • source-recharge
  • source-twilio
  • source-gitlab
  • source-onesignal
  • source-quickbooks-singer
  • source-freshcaller
  • source-notion
  • source-facebook-marketing
  • source-braintree
  • source-amazon-sqs
  • source-asana
  • source-zendesk-talk
  • source-airtable
  • source-appsflyer
  • source-github
  • source-amazon-seller-partner
  • source-tplcentral
  • source-monday

@davydov-d
Merge branch 'master' into ddavydov/source-acceptance-tests-fix-backw…
e6b1b3f 
…ards-compatibility-test
@davydov-d
SATs: fix changelog
80819ed
@davydov-d
Merge branch 'ddavydov/source-acceptance-tests-fix-backwards-compatib…
5061edc 
…ility-test' of github.com:airbytehq/airbyte into ddavydov/source-acceptance-tests-fix-backwards-compatibility-test
@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-klaviyo
  • source-commercetools
  • source-surveymonkey
  • source-appsflyer
  • source-monday
  • source-confluence
  • source-harvest
  • source-google-search-console
  • source-braintree
  • source-airtable
  • source-tplcentral
  • source-pardot
  • source-sentry
  • source-lever-hiring
  • source-quickbooks-singer
  • source-linnworks
  • source-recharge
  • source-mailgun
  • source-plaid
  • source-zendesk-sunshine
  • source-freshcaller
  • source-iterable
  • source-paystack
  • source-amazon-sqs
  • source-greenhouse
  • source-zendesk-talk
  • source-openweather
  • source-asana
  • source-prestashop
  • source-zenloop
  • source-retently
  • source-sendgrid
  • source-google-ads
  • source-posthog
  • source-drift
  • source-gitlab
  • source-facebook-pages
  • source-twilio
  • source-facebook-marketing
  • source-lemlist
  • source-strava
  • source-onesignal
  • source-amplitude
  • source-youtube-analytics
  • source-instagram
  • source-mailchimp
  • source-delighted
  • source-chargebee
  • source-cart
  • source-amazon-ads
  • source-pipedrive
  • source-okta
  • source-salesforce
  • source-freshservice
  • source-pinterest
  • source-amazon-seller-partner
  • source-freshsales
  • source-azure-table
  • source-notion
  • source-salesloft
  • source-github
  • source-outreach

@davydov-d
Copy link
Collaborator Author

davydov-d commented Sep 12, 2022 •
edited by github-actions bot

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/3035294642


Connector Did it publish? Were definitions generated?
bases/source-acceptance-test ✅ ❌

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

@davydov-d davydov-d merged commit f46aaff into master Sep 12, 2022
@davydov-d davydov-d deleted the ddavydov/source-acceptance-tests-fix-backwards-compatibility-test branch September 12, 2022 07:38
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Sep 12, 2022
Merged
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
@davydov-d @alafanechere @robbinhan
SATS: fix backwards compatibility (airbytehq#16429)
6667c3e 
* SATS: fix backwards compatibility

* airbytehq#16429 source acceptance tests: modify validation for specs only

* airbytehq#16429 source acceptance tests: add typing

* SATs: fix changelog

Co-authored-by: Augustin <augustin.lafanechere@gmail.com>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@davydov-d @alafanechere @jhammarstedt
SATS: fix backwards compatibility (airbytehq#16429)
e9de00a 
* SATS: fix backwards compatibility

* airbytehq#16429 source acceptance tests: modify validation for specs only

* airbytehq#16429 source acceptance tests: add typing

* SATs: fix changelog

Co-authored-by: Augustin <augustin.lafanechere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroslopez pedroslopez pedroslopez approved these changes

@alafanechere alafanechere alafanechere approved these changes

@sherifnada sherifnada sherifnada approved these changes

@lazebnyi lazebnyi lazebnyi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@davydov-d @pedroslopez @alafanechere @sherifnada @lazebnyi