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 Stripe : Remove Updated field from schemas #4980

Conversation

yevhenii-ldv
Copy link
Contributor

What

closes #4921.

How

Describe the solution

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in the connector's spec
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste 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.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Documentation updated
    • README.md
    • docs/SUMMARY.md if it's a new connector
    • Created or updated reference docs in docs/integrations/<source or destination>/<name>.
    • Changelog in the appropriate page in docs/integrations/.... See changelog example
    • docs/integrations/README.md contains a reference to the new connector
    • Build status added to build page
  • 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
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

Connector Generator checklist

  • 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.

ykurochkin added 2 commits July 26, 2021 17:52
@github-actions github-actions bot added the area/connectors Connector related issues label Jul 26, 2021
@avida
Copy link
Contributor

avida commented Jul 26, 2021

Please rebase this change on latest master. Your SAT run would fail w/o it.

ykurochkin added 4 commits July 26, 2021 19:33
…/source-stripe-use-updated-instead-of-created-for-cursor-values

� Conflicts:
�	airbyte-integrations/connectors/source-stripe/integration_tests/non_disputes_events_catalog.json
�	airbyte-integrations/connectors/source-stripe/integration_tests/non_invoice_line_items_catalog.json
�	airbyte-integrations/connectors/source-stripe/integration_tests/subscription_catalog.json
�	airbyte-integrations/connectors/source-stripe/source_stripe/schemas/customers.json
�	airbyte-integrations/connectors/source-stripe/source_stripe/schemas/fullschema.json
…/source-stripe-use-updated-instead-of-created-for-cursor-values

� Conflicts:
�	airbyte-integrations/connectors/source-stripe/integration_tests/non_disputes_events_catalog.json
�	airbyte-integrations/connectors/source-stripe/integration_tests/non_invoice_line_items_catalog.json
…/source-stripe-use-updated-instead-of-created-for-cursor-values
@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Jul 27, 2021

/test connector=connectors/source-stripe

🕑 connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/1070514676
✅ connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/1070514676

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jul 27, 2021
}
}
},
"json_schema": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

In normalization, we expect streams in catalog to always have json_schema.properties otherwise we raise exceptions:

properties = get_field(get_field(stream_config, "json_schema", message), "properties", message)

Why are we sending streams with empty schemas for?

Copy link
Contributor Author

@yevhenii-ldv yevhenii-ldv Jul 28, 2021

Choose a reason for hiding this comment

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

@ChristopheDuong
In this case, we use configured_catalog.json for Acceptance Tests, and in order not to duplicate the scheme once again, a processing has been created that pulls up the scheme for each stream from the streams/ directory.
You can see it here.

In your case, we get a catalog that is located in the f"resources/{catalog_file}.json" file. I am not very competent in this code, since I have not worked with it, but I suspect that these catalogs were created using the discover command and placed in the resources/ directory, and json_schema is always present there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i see, that's how the tests are done here, ok!

I was just worried that sources would produce streams with empty properties as it already happened in the past as shown recently in this thread:
https://airbytehq.slack.com/archives/C01MFR03D5W/p1627044823226100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No no, this is a different case. This practice of using catalogs without json_schema for Acceptance Tests is not new, it has been used for about month for sure with different connectors.

@yevhenii-ldv
Copy link
Contributor Author

yevhenii-ldv commented Jul 28, 2021

/publish connector=connectors/source-stripe

🕑 connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/1074819506
✅ connectors/source-stripe https://github.com/airbytehq/airbyte/actions/runs/1074819506

@yevhenii-ldv yevhenii-ldv merged commit f538a63 into master Jul 28, 2021
@yevhenii-ldv yevhenii-ldv deleted the ykurochkin/source-stripe-use-updated-instead-of-created-for-cursor-values branch July 28, 2021 12:21
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Stripe : Use Updated instead of Created for cursor values
6 participants