CRAYSAT-1917: Fix jinja2 rendering of boot_sets data in sat bootprep
#280
Conversation
|
Testing output on starlord- https://gist.github.com/annapoorna-s-alt/981715de25d248527c7200d50398c731 |
|
Adding unit tests are pending |
|
Core functionality is tested. Need to update the unit tests for the same. @annapoorna-s-alt - Would take the ticket forward, as she has tested the draft PR |
There was a problem hiding this comment.
Looks good so far, and testing output looks good. I suggested to Annapoorna that she could also test that other fields in the boot_sets support Jinja2 rendering since this has been added by the changes to jinja_rendered.
I will review again once that testing is done and once unit tests are added.
|
Output of testing other fields in the boot_sets support Jinja2 rendering - https://gist.github.com/annapoorna-s-alt/981715de25d248527c7200d50398c731#file-expand-other-fields-with-jinja |
annapoorna-s-alt
force-pushed
the
CRAYSAT-1917-fix-jinja2-rendering
branch
from
dcff8f2
to
70fa8fa
Compare
Make the following fixes: - Remove the `boot_set` property from `BaseInputItem`. It is not appropriate there, and it is redundant with `boot_sets` property that already exists in the `InputSessionTemplate` - Modify the `jinja_rendered` decorator to recursively render more complex objects like lists and dictionaries. I think this is safe, but we should consider any edge cases more carefully. - Remove the now unnecessary code that does a one-off Jinja2 rendering of `rootfs_provider_passthrough` in the `boot_sets`. Could still use unit test enhancements that test this new ability to render fields in the boot_sets. Test Description: Tested on a simple bootprep input file that used a variable in the `rootfs_provider_passthrough` field of a boot set in a BOS session template.
annapoorna-s-alt
force-pushed
the
CRAYSAT-1917-fix-jinja2-rendering
branch
from
70fa8fa
to
71ef427
Compare
annapoorna-s-alt
requested review from
vinanti-p
and removed request for
shivaprasad-metimath
Add unit tests for usage of the `jinja_rendered` decorator on properties of the `BaseInputItem`. Modify these unit tests to simplify them slightly and to ensure that the `SomeTestItem` class calls the `BaseInputItem` constructor, so that it sets the `jinja_env` instance attribute, which is used by the `jinja_rendered` decorator. Test Description: Unit tests pass.
There was a problem hiding this comment.
The added unit test does not test jinja_rendered, which is what is being changed. I have opened a PR to this branch to add such unit tests. Please take a look: #282
This testing looks good. |
Update the unit tests of the InputSessionTemplate class to include tests that check for proper Jinja2 template rendering of the boot_sets field.
…-unit-tests CRAYSAT-1917: Add unit tests for `jinja_rendered` decorator
|
/backport release/3.32 |
|
Backporting into branch release/3.32 was successful. New PR: #284 |
Summary and Scope
Make the following fixes:
boot_setproperty fromBaseInputItem. It is not appropriate there, and it is redundant withboot_setsproperty that already exists in theInputSessionTemplatejinja_rendereddecorator to recursively render more complex objects like lists and dictionaries. I think this is safe, but we should consider any edge cases more carefully.rootfs_provider_passthroughin theboot_sets.Could still use unit test enhancements that test this new ability to render fields in the boot_sets.
Issues and Related PRs
List and characterize relationship to Jira/Github issues and other pull requests. Be sure to list dependencies.
Testing
List the environments in which these changes were tested.
Tested on:
Test description:
How were the changes tested and success verified? If schema changes were part of this change, how were those handled in your upgrade/downgrade testing?
Tested on a simple bootprep input file that used a variable in the
rootfs_provider_passthroughfield of a boot set in a BOS session template.Risks and Mitigations
Low
Pull Request Checklist