-
Notifications
You must be signed in to change notification settings - Fork 313
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
[Resolves #937] Reset Jinja2 variable cache between config files #938
[Resolves #937] Reset Jinja2 variable cache between config files #938
Conversation
While reading config files, Sceptre caches the variables used by Jinja. If a template is missing a variable, one from a different StackGroup is used instead.
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.
Thank you @NathanWilliams this has been a really subtle one that is catching folk out.
I think this resolves #813 and #889
I wonder if we could structure a test with this so that we can be sure it fixes the issue?
@ngfgrant Sure. I have been on a break, so I'll try and fit it in in the next day or so |
@zaro0508 I spent quite a bit of time on this and I believe this is a valid test case: --- a/tests/test_config_reader.py
+++ b/tests/test_config_reader.py
@@ -621,6 +621,32 @@ class TestConfigReader(object):
assert result == {"key": "value"}
+ def test_render__existing_config_file__stack_group_config_leaks(self):
+ with self.runner.isolated_filesystem():
+ project_path = os.path.abspath("./example")
+ config_dir = os.path.join(project_path, "config")
+ directory_path = os.path.join(config_dir, "configs")
+
+ os.makedirs(directory_path)
+
+ basename = "existing_config.yaml"
+ stack_group_config = {}
+
+ test_config_path = os.path.join(directory_path, basename)
+ test_config_content = "key: value\n"
+
+ with open(test_config_path, "w") as file:
+ file.write(test_config_content)
+
+ config_reader = ConfigReader(self.context)
+ config_reader.templating_vars["someone_elses"] = "config"
+ config_reader.templating_vars["stack_group_config"] = {"foo": "bar"}
+ config_reader.templating_vars["var"] = {"baz": "qux"}
+
+ _ = config_reader._render("configs", basename, stack_group_config)
+
+ assert config_reader.templating_vars == {"someone_elses": "config", "stack_group_config": {"foo": "bar"}, "var": {"baz": "qux"}}
+
def test_render__invalid_jinja_template__raises_and_creates_debug_file(self):
with self.runner.isolated_filesystem():
project_path = os.path.abspath("./example") Where I have failed is to reproduce the original issue in the first place. The original author's set up is quite different to what I'm familiar with and I can't use it to reproduce anything. I do suspect this bug still exists even 4 years later! If so, we really should merge this. |
I have adopted this stale PR |
From https://github.com/NathanWilliams I am adopting this stale PR: #938 --- Fix for: #937 This resets the Jinja2 templating_vars in config/reader.py to prevent variables leaking across StackGroups. Without this patch applied, the test case added would fail and show param1 from stack_group_config1 has leaked into stack_group2: ``` E Full diff: E { E 'j2_environment': {}, E - 'param1': 'value1', E 'param2': 'value2', E 'var': 'initial_value', E } ``` Nathan's fix is shown to prevent this unintended behaviour.
Fix for: #937
This resets the Jinja2
templating_vars
inconfig/reader.py
to prevent variables leaking across StackGroups.PR Checklist
[Resolve #issue-number]
.make test
) are passing.make lint
) checks.and description in grammatically correct, complete sentences.
Approver/Reviewer Checklist
Other Information
Guide to writing a good commit