-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Polygon Stocks #18842
🎉 New Source: Polygon Stocks #18842
Conversation
Issue related to this PR : |
@Harshil0512 please check your commits there is something wrong with it. |
@marcosmarxm may I know what is wrong with the commit ? is there something wrong with the files which I created or something with the files belongs to your repository updated due to my commit? |
You're pushing files that don't make part of the pull request goal. Take a look. |
Okay let me see and update it |
@marcosmarxm can you please check the files now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Harshil0512, Marcos from Airbyte here 👋 . We received more than 25 new contributions along the weekend. One is yours 🎉 thank so much for! Our team is limited and maybe the review process can take longer than expected. As described in the Airbyte's Hacktoberfest your contribution was submitted before November 2nd and it is eligible to win the prize. The review process will validate other requirements. I ask to you patience until someone from the team review it.
Because I reviewed some contributions for Hacktoberfest so far I saw some common patterns you can check in advance:
- Make sure you have added connector documentation to
/docs/integrations/
- Remove the file
catalog
from/integration_tests
- Edit the
sample_config.json
inside/integration_tests
- For the
configured_catalog
you can use onlyjson_schema: {}
- Add title to all properties in the
spec.yaml
- Make sure the
documentationUrl
in thespec.yaml
redirect to Airbyte's future connector page, eg: connector Airtable thedocumentationUrl: https://docs.airbyte.com/integrations/sources/airtable
- Review now new line at EOF (end-of-file) for all files.
If possible send to me a DM in Slack with the tests credentials, this process will make easier to us run integration tests and publish your connector. If you only has production keys, make sure to create a bootstrap.md explaining how to get the keys.
Okat let me review the files once again. Thank you. |
Hello! I'm going to be out of the office this Friday and won't be able to review your contribution again today, I return next Monday. So far, most contributions look solid and are almost done to be approved. As said in Chris' comment all contributions made before 2-November are eligible to receive the prize and have 2 weeks to merge the contributions. But I ensure next week we're going to have your contribution merged. If you have questions about the implementation you can send them in Sorry the inconvenience and see you again next week, thank you so much for your contribution! |
Sure No Problem @marcosmarxm |
I performed the changes mentioned in the review changes |
Are you there @marcosmarxm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
$options: | ||
name: "stock_api" | ||
primary_key: "id" | ||
path: "/v2/aggs/ticker/{{ config['stocksTicker'] }}/range/{{ config['multiplier'] }}/{{ config['timespan'] }}/{{ config['from'] }}/{{ config['to'] }}?adjusted={{ config['adjusted'] }}&sort={{ config['sort'] }}&limit=120&apiKey={{ config['apiKey'] }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please overwrite the request parameter component.
Please implement the DateStreamSlicer to enable incremental syncs.
Always sort by desc to read first old to newest data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still to be done I'll finish this soon.
streams: | ||
- "*ref(definitions.stock_api_stream)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to add only this endpoint? I saw other endpoints in the API docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes as I thought it is the most important end point as per my point of view
from: | ||
type: string | ||
description: The beginning date for the aggregate window. | ||
examples: | ||
- "2020-10-14" | ||
to: | ||
type: string | ||
description: The target date for the aggregate window. | ||
examples: | ||
- "2020-10-14" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change from
to start_date
and to
to end_date
is more used by Airbyte's connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this
- IBM | ||
- MSFT | ||
multiplier: | ||
type: integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add title to your parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this
- multiplier | ||
- timespan | ||
- from | ||
- to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to
or end_date
is not required if you implement the datestream slicer.
Okay I'll work on that and will keep you updated |
@Harshil0512 I had a problem when pushing some change to your branch :( I'll finish your work at #19504 Thanks so much for the contribution @RealChrisSean same problem real, sorry!!! |
What
The Following PR adds a Low Code CDK Source for Polygon-Stocks-API provided by https://polygon.io/
How
The PR uses the configuration based CDK and implements full refresh syncs of the following streams :
Recommended reading order
spec.yaml
polygon_stock_api.yaml
streams
🚨 User Impact 🚨
No Changes To Existing Code
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.