-
Notifications
You must be signed in to change notification settings - Fork 185
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
Replace weather api example with github in create a pipeline walkthrough #1351
Replace weather api example with github in create a pipeline walkthrough #1351
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Almost good! one more explanation needed
8c48a76
to
893c68b
Compare
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.
Not sure if Github API is a good example for this page, because we have all code in the init template. imo weatherapi was more informative, because we build the pipeline from scratch
@AstrakhantsevaAA the problem with WeatherAPI is that it requires custom pagination which will be out of scope for this task, so the simple solution here is to go with GitHub API |
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.
Great work! I've added some comments and fixes below.
Co-authored-by: Anton Burnashev <[email protected]>
Co-authored-by: Anton Burnashev <[email protected]>
Co-authored-by: Anton Burnashev <[email protected]>
Co-authored-by: Anton Burnashev <[email protected]>
2919ecb
to
cb43a10
Compare
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.
Great work, @sultaniman! Thanks for putting this together!
@burnash thanks! I think you have to merge this because I don't have permissions to merge on failing CI items. |
Thanks @sultaniman the CI should be fixed in #1364 |
This PR updates a walkthrough (Create a pipeline): Create a pipeline
Closes: #1350