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

Fix loading main_config again during discovery of dynamic configurations #450

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

cimnine
Copy link
Collaborator

@cimnine cimnine commented Feb 23, 2021

Related Issue: #445

New Behavior

The main configuration files, configuration.py and ldap_config.py, were accidentally loaded twice: The initial load was hard-coded and correct. But during the discovery of the dynamic, user-contributed configuration files (like extra.py or whatever.py), the main configuration files were discovered as well and loaded again.
They have thereby overwritten all settings in files that were loaded before them.

An example, given the file a_config.py:

# a_config.py
BANNER_LOGIN = "Hello Mr. Anderson. 💊"

The old behavior was:

  1. Load configuration.py
  2. Discover a_config.py and configuration.py
  3. Set the value of BANNER_LOGIN to what is found in a_config.py.
  4. Set the value of BANNER_LOGIN to what is found in configuration.py,
    because it's alphabetically after a_config.py and falsely discovered again.
  5. Done.

The new behavior is – as it should be:

  1. Load configuration.py
  2. Discover a_config.py and configuration.py, but ignore configuration.py, as it's the main config file.
  3. Set the value of BANNER_LOGIN to what is found in a_config.py.
  4. Done.

Thanks to @tobiasge for discovering this bug.

Contrast to Current Behavior

The described bug has now been fixed. During the discovery for dynamic configurations,
when loading each of the discovered configuration files, the main_config file is now excluded.

Discussion: Benefits and Drawbacks

The intended behavior is achieved: All values of the main configuration files can be overwritten with values in arbitrarily named custom configuration files.

Changes to the Wiki

n/a

Proposed Release Note Entry

## Fix Custom Configuration Discovery #450

There was a bug which prevented that certain custom configuration files were not loaded.
This was the case, when the custom configuration file's name was alphabetically before the main configuration file's name.
I.e. `a_configuration.py` is alphabetically before `configuration.py`. With this file, `a_configuation.py`, it was therefore not possible to overwrite values which are already declared in `configuration.py`. (The same holds true for `ldap_config.py`, but it's worse, because the letter L is relatively late in the alphabet.)

See #450 for more details.

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

Before, the main_config (e.g. 'configuration.py' or 'ldap_config.py') were loaded twice.
The first load was hard-coded and correct.
The second load was during the discovery phase for dynamic configurations.
This has now been fixed. During the discovery for dynamic configurations,
the main_config file is now excluded.

Thanks @tobiasge for discovering this bug.
@cimnine cimnine added the bug This issue describes a confirmed bug. label Feb 23, 2021
@cimnine cimnine added this to the 1.1.0 milestone Feb 23, 2021
@cimnine cimnine self-assigned this Feb 23, 2021
@cimnine cimnine merged commit 6cd156f into develop Feb 24, 2021
@cimnine cimnine deleted the FilterMainConfig branch February 24, 2021 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants