Only compile the template contents if they evaluate to True #33679
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When getting all the top files for the various environments, those environments without a top file will return ``False`` when we attempt to cache the file. Passing a boolean to ``compile_template()`` will result in an error being logged, since we are invoking the function incorrectly (it expects the template data to be a string). Since a ``False`` return from caching the file means that it does not exist, this commit will only run ``compile_template()`` when the contents evaluate to ``True``. Resolves saltstack#33424.
|
Yes, this looks good to me, your logic is sound |
|
@terminalmage Could you look at the test failures? I think I wrote some of those so I can help if needed. |
|
Looking now. |
Rather than excluding them from the get_tops() return data at all, just populate them with empty dictionaries. This makes it easier to unit test things like merge order, while still being functionally the same as exlcluding the saltenv from get_tops() altogether.
|
Should be fixed. Instead of leaving the environments out of the return dict altogether, I'm just appending empty dictionaries to them. It seems from the failing unit tests that this was the original intent, and doing so both does not impact the highstate logic while making it easier to unit test things like top file merging. |
|
Solid. +1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When getting all the top files for the various environments, those
environments without a top file will return
Falsewhen we attempt tocache the file. Passing a boolean to
compile_template()will resultin an error being logged, since we are invoking the function incorrectly
(it expects the template data to be a string).
Since a
Falsereturn from caching the file means that it does notexist, this commit will only run
compile_template()when thecontents evaluate to
True.Resolves #33424.
@thatch45 Is this a sane fix? Is there any potential breakage that could occur
by leaving environments lacking a top file (or I guess, those with top files of
zero length) out of the return data from
get_tops()? I can't see any.