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.

Sign up for GitHub

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

Jump to bottom

Add jinja preprocessing to YamlTemplate #1500

Merged
merged 1 commit into from May 13, 2024
Merged

Add jinja preprocessing to YamlTemplate #1500

merged 1 commit into from May 13, 2024

Conversation

Polber
Copy link
Contributor

@Polber Polber commented May 2, 2024

Adds ability to perform jinja preprocessing to Beam YAML jobs run on YamlTemplate until the functionaiity is added to vanilla Beam YAML in Beam 2.47.0

Adds template parameter --jinja_variables

@Polber Polber requested a review from damccorm May 2, 2024 00:46
@Polber Polber self-assigned this May 2, 2024
@codecov Codecov
Copy link

codecov bot commented May 2, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.18%. Comparing base (8936b1d) to head (b961be5).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1500      +/-   ##
============================================
+ Coverage     39.88%   40.18%   +0.29%     
- Complexity     2782     2792      +10     
============================================
  Files           750      737      -13     
  Lines         42568    42768     +200     
  Branches       4555     4578      +23     
============================================
+ Hits          16980    17186     +206     
+ Misses        24121    24101      -20     
- Partials       1467     1481      +14
Components Coverage Δ
spanner-templates 56.79% <46.86%> (+0.07%) ⬆️
spanner-import-export 65.50% <ø> (-0.07%) ⬇️
spanner-live-forward-migration 63.17% <94.73%> (+1.93%) ⬆️
spanner-live-reverse-replication 42.95% <93.61%> (+0.42%) ⬆️
spanner-bulk-migration 69.97% <45.05%> (-2.03%) ⬇️

see 35 files with indirect coverage changes

damccorm
damccorm reviewed May 2, 2024
View reviewed changes
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Comments are minor, overall looks good to me.

cc/ @robertwb

python/src/main/python/yaml-template/main.py Outdated Show resolved Hide resolved
python/src/main/python/yaml-template/main.py Outdated
Comment on lines 56 to 57
pipeline_yaml = os.linesep.join(
[s for s in pipeline_yaml.splitlines() if s.strip()])
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside here is that we lose the original parameters in our pipeline options/display data. I think this is fine, probably a no-op for this PR, but left a comment in https://github.com/apache/beam/pull/30976/files#r1588005108

robertwb
robertwb reviewed May 2, 2024
View reviewed changes
python/src/main/python/yaml-template/main.py Outdated Show resolved Hide resolved
python/src/main/python/yaml-template/main.py Outdated Show resolved Hide resolved
python/src/test/resources/YamlTemplateIT.yaml Outdated Show resolved Hide resolved
@Polber Polber force-pushed the jkinard/jinja branch 2 times, most recently from f715a98 to f6e9709 Compare May 7, 2024 14:41
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 7, 2024
@Polber
Copy link
Contributor Author

Polber commented May 7, 2024

@damccorm @robertwb After reconsidering, I decided just to copy over the main from apache/beam#30976 for the time being. There were too many issues involving overridden pipeline args, and it makes more sense to me to just copy over the same logic that will be in Beam 2.57.0 instead of trying to stitch pieces together. Once 2.57.0 is release, assuming the jinja PR is merged, I will revert the template back to how it was before.

damccorm
damccorm reviewed May 13, 2024
View reviewed changes
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Could we add a GitHub issue to track the work to remove these pieces before proceeding? Otherwise LGTM

@damccorm damccorm added the Google LGTM Approval of a pull request to be merged into the repository label May 13, 2024
@Polber Polber mentioned this pull request May 13, 2024
Open
@Polber
Copy link
Contributor Author

Polber commented May 13, 2024

Could we add a GitHub issue to track the work to remove these pieces before proceeding? Otherwise LGTM

#1554

@Polber
Add jinja preprocessing to YamlTemplate
b961be5 
Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>
@copybara-service copybara-service bot merged commit 5597215 into GoogleCloudPlatform:main May 13, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertwb robertwb robertwb left review comments

@damccorm damccorm damccorm approved these changes

Assignees

@Polber Polber

Labels
Google LGTM Approval of a pull request to be merged into the repository size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Polber @robertwb @damccorm