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

Load globs in alhpebetical order to prevent undefined behavior #40

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Jul 2, 2018

Previously files were not sorted at all (using the order at which entries appear in the filesystem). With this approach every file needs to be capable of being loaded in any order- something I woudln't expect users to test for. Sorting files by name-order allows for index config files like 010-base.py and 030-secrets.py to load procedurally and deterministically, and reduces complexity when in use.

@coveralls
Copy link

coveralls commented Jul 2, 2018

Coverage Status

Coverage remained the same at 93.182% when pulling 9174e61 on ava-dylang:deterministic-directory-loading into ad67800 on sobolevn:master.

@sobolevn
Copy link
Member

sobolevn commented Jul 3, 2018

@ava-dylang I appreciate your contribution!

Can you please point out several things to me:

  1. is it possible to import these files? 010-some.py and others?
  2. why don't you just import them as named imports? In my vision globs are only used when order should not matter at all.

Thanks!

@ghost
Copy link
Author

ghost commented Jul 6, 2018

  1. The test cases imported them fine
  2. This requires I know the name of the settings files in advance (defeating the purpose of a settings.d directory where an operator can configure an application as desired), I could glob them myself-sort them myself-and import them myself but at that point I don't have a reason to use this library as I already can make assumptions about namespaces in being defined or not and precidance if lists items. Import order will always matter. Loading config files in a random order will result in things like INSTALLED_APPS being broken for modules that have to be loaded first or last.

People is Django in other ways than to host first party proprietary apps where they have full control over settings mattering- using random open source Django apps is often enough of a headache to repair to be operable without the filesystem order of a glob making build artifacts unreliable etc.

I don't understand why pushback on this change except backwards comparability concerts, however defiantly undefined-behavior is also definitely a bug capable of arbitrary code execution, so I'm fully comfortable considering this change a minor bugfix.

If this is untenable, I'm also comfortable producing this change as an optional behavior that has to be enabled.
Thank you for your time and wonderful settings loader idea! I wouldn't have figured out the scope merging trick without having seen it here!!
Have a nice day please
🙇

@sobolevn
Copy link
Member

sobolevn commented Jul 7, 2018

I am not against this feature at all, I am just trying to figure out some things that I did not understand from the first time.

I see some technical issues that should be fixed before this is merged.

Technical issues

I was talking about importing these files with python:

» cat 01-test.py
VALUE = 12
                       
» python        
Python 3.6.5 (default, Mar 30 2018, 06:41:49) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from 01-test import VALUE
  File "<stdin>", line 1
    from 01-test import VALUE
          ^
SyntaxError: invalid token

Module names:

  • can not start with numbers
  • can not contain -, only _ is allowed

Idealogical issues

I also do not want to enforce the way of relying on glob order in the README. Since I believe that order should be defined as named imports. But having ordered glob is a good thing.

We already have a note about globs , so we can just edit this section. Stating that actually you can now rely on the order.

@ghost
Copy link
Author

ghost commented Jul 11, 2018

RE Tech;
as per the file names, if you want them to be named with underscore that is 100% okay with me

I prefer config files (well all files names really) to be hyphenated, and checking my system with a quick grep and on /etc; I have about ~2500 files with - delimiters, and ~1200 files with _ delimiters there -- so I would say at a system configuration level hypens are more common -- this is entirely irrelevant though, and just me trying to promote consistency.

The reason the file name doesn't matter in this case comes down to the way this module is loading the file by-file-name coupled with managing the file's loaded scope, doing so like this module does disregards functional criteria of python file names having to be valid namespaces in the current context scope (where hypen is reserved for the subtraction operator). I hope that makes sense, Honestly i've never seen python modules loaded in a way that gets around this constraint, so I think this is really cool!! Or to propose how this module acts another way, when this loads a file, its very similar to if one had concatenated all the files and saved it and imported it ((never do that btw, what this module does is far far better for state management and consistency))

RE Idea,
if this module is intended to load files in an entirely reandom order based on file system super blocks and inode allocation, mutable via defragmentation -- I think i've made clear how I can't endorse that

if this module want to imply that whoever is operating the django project that loads settings files always and forever has direct development access to the django modules their loading (Including an understanding of python development, a familiarity with django, a background in programming, a way to test changes for production etc) -- I can undestand that, but I feel it weakens django for operation, like how I want to use it -- where one django host is loading a handful of open source django modules, each of which need thier own config, some of which demand order loading, and all of which, require mutating the same INSTALLED_APPS variable over and over and over without error
-- I think i'm just repeating myself at this point, I'm 100% ok closing this issue and using this module with the following boilerplate in my settings file:

split_settings.tools.include(sorted(glob.glob('settings.d/*')))

This way I'm explicitly declaring all the files I want loaded (like you say this module's ideology expects) in the order I want to load them (by an ordered set of partial files in a directory, allowing a system application to be configured by the system operators and package managers without conflict)

Thank you for your time and thoughts on this topic

@ghost ghost closed this Jul 11, 2018
This pull request was closed.
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.

None yet

3 participants