-
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
Basic yaml-defined provider. #31684
Basic yaml-defined provider. #31684
Conversation
R: @Polber |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31684 +/- ##
============================================
+ Coverage 71.37% 71.40% +0.02%
Complexity 1474 1474
============================================
Files 900 900
Lines 114202 114306 +104
Branches 1076 1076
============================================
+ Hits 81515 81616 +101
- Misses 30659 30662 +3
Partials 2028 2028
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ping @Polber |
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.
Left a couple small comments, but overall I think it looks good and the syntax makes sense to me
except ImportError: | ||
pass |
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.
Should there be a warning that validation is unable to be performed?
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.
Good catch.
if not isinstance(body, str): | ||
body = yaml.safe_dump(SafeLineLoader.strip_metadata(body)) | ||
templated = ( # keep formatting | ||
jinja2.Environment(undefined=jinja2.StrictUndefined) |
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 should probably use the same logic as main (i.e. allows GCS file includes)
jinja2.Environment(undefined=jinja2.StrictUndefined) | |
jinja2.Environment(undefined=jinja2.StrictUndefined, loader=_BeamFileIOLoader()) |
Maybe it would make sense to make a standalone function for loading jinja templates to help keep functionality synced
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.
Yeah, I'll do that.
This allows one to define new transforms with YAML that can be imported and used in other pipelines.
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.
PTAL
This allows one to define new transforms with YAML that can be imported and used in other pipelines.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.