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.

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

[Resolve #937] Reset Jinja2 variable cache between config files #1472

Conversation

alex-harvey-z3q
Copy link
Contributor

@alex-harvey-z3q alex-harvey-z3q commented Jun 4, 2024

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.

PR Checklist

  • Wrote a good commit message & description [see guide below].
  • Commit message starts with [Resolve #issue-number].
  • Added/Updated unit tests.
  • Added/Updated integration tests (if applicable).
  • All unit tests (poetry run tox) are passing.
  • Used the same coding conventions as the rest of the project.
  • The new code passes pre-commit validations (poetry run pre-commit run --all-files).
  • The PR relates to only one subject with a clear title.
    and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

  • Before merge squash related commits.

Other Information

Guide to writing a good commit

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.
@alex-harvey-z3q alex-harvey-z3q changed the title [Resolves #937] Reset Jinja2 variable cache between config files [Resolve #937] Reset Jinja2 variable cache between config files Jun 10, 2024
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

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

The issue for this hints that using a non deterministic set is the problem...

If a variable is omitted in one stack group's config, 
the incorrect value from another group is used.
(Note: Sceptre uses a set for its file list, which is non-deterministic,
as such the following bug will only be seen on some runs)

I'm wondering if the better fix is to use an more deterministic data structure, like a ordered set?

@alex-harvey-z3q
Copy link
Contributor Author

I'm wondering if the better fix is to use an more deterministic data structure, like a ordered set?

@zaro0508 That's not a fix for this bug but it would have assisted the original team who reported the bug in being able to consistently reproduce it. The nature of the bug here is simply that keys belonging to other stack groups leaked into the configs of stack groups they did not belong to and this would occur with or without ordering. Maybe track a separate issue of making the code a bit more predictable by using ordered sets where applicable?

@zaro0508 zaro0508 merged commit 20801f8 into Sceptre:master Jun 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants