-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Goldcast #38786
⭐ New Source: Goldcast #38786
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Overall, this looks great and I'd be happy to merge this once CI passes. This should then be available promptly once it populates to registry.
I left some suggestions, and I'd love you to act on them if you have time.
|
||
|
||
# Declarative Source | ||
class SourceGoldcast(YamlDeclarativeSource): |
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.
In theory you can drop this file and just inherit from YamlDeclarativeSource in main.py
, but it's okay either way
@@ -0,0 +1,191 @@ | |||
{ |
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 would prefer to inline schema in the yaml manifest, but again, can come later. Esp if you're interested in making more sources!
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.
@natikgadzhi , Its done as well ! Would you like me to delete these json schemas ? Is it not required ?
If you can allow me to make changes to this PR, I'm happy to make some changes myself. |
Co-authored-by: Natik Gadzhi <natik@respawn.io>
Head branch was pushed to by a user without write access
@FVidalCarneiro I expec that this will ship today — as long as the tests are green, I'll merge. |
Regarding this point @natikgadzhi , I would gladly share you edit access but I had this problem in another PR where I tried to follow (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests) and think it is not possible. Here is the link to the git discussion (#34166 (comment)). |
Merging. This should show up on the catalog in an hour or so |
@natikgadzhi , thank you for your support with this. Happy to have our contributions integrated 😄 |
Great work adding a new source! We'll make this very easy in the future, so the next time it doesn't take you months of kicking us to review. |
What
This Pull Request aims in responding to issue (#38785) by proposing a first version of the Goldcast source connector.
How
This PR adds 8 base streams of the Goldcast API:
This connector was developed using the low-code development framework.
Schemas of the ingested objects are defined in the
schemas/
directory.Review guide
User Impact
User will have access to 8 streams of a new stream.
Attention: It is worth mentioning that due to field limitations on Goldcast side, only full append stream ingestion modes are supported. This is no cursor field available yet for these API objects.
Can this PR be safely reverted and rolled back?