-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[tech debt] Avoid recursive include of DEFAULT_SETTINGS, sanity test #13236
Conversation
1df7f8e
to
af99df7
Compare
DEFAULTS_SNAPSHOT[setting] = copy.deepcopy(getattr(this_module, setting)) | ||
|
||
del local_vars | ||
del this_module |
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.
I wanted to del
these because we do use dir
on the settings module in our settings wrapper:
https://github.com/ansible/awx/blob/devel/awx/conf/settings.py#L474
and I just don't want to pollute that namespace with more variables that should be internal to logic inside of settings. It would be impractical to add an __ALL__
to this module. Also, the original recursive copy here was because of lack of awareness of what things hanging out in locals(). We avoid copying lowercase vars in the loop, but I prefer to explicitly exclude them from the namespace, as they should not be public.
af99df7
to
64dc22f
Compare
Integration tests are passing, so I'm happy with this myself now. |
cb6391b
to
04e43d2
Compare
04e43d2
to
e317643
Compare
@john-westcott-iv I believe I updated based on all your review comments. How does it look now? |
e317643
to
eaa7c0a
Compare
SUMMARY
I told @john-westcott-iv this was going to bother me, but I wasn't going to make a PR to change it until I saw this failure.
Wait, what? The snapshot value for
DEFAULTS_SNAPSHOT
doesn't match? That doesn't make any sense. We got that snapshot value from the snapshot dictionary! Did this dict save a copy of itself... inside itself?Yes.
ISSUE TYPE
COMPONENT NAME