Fix config hierarchy #3512
Merged
Fix config hierarchy #3512
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eter and skip the db stuff Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
…or the merging functionality Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| from sqlalchemy import and_, update | ||
| import json | ||
| import copy | ||
| from typing import List, Any, Optional |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused List imported from typing (unused-import)
Contributor
Author
|
I also tested this by running Augur before and after this change. Before I observed that the augur_collection_monitor task was failing every 30 seconds (as seen in Flower) with the same exception about the missing config key. Afterwards, i see these tasks succeeding again |
MoralCode
added
the
ready
1 task
Signed-off-by: Adrian Edwards <17362949+MoralCode@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR corrects a bug that was introduced with the config hierarchy PR in #3378
The bug was that I was using python's builtin
update()function to attempt to merge the configs from different sources together (represented as nested dicts). However,update()only updates one level of the dictionary, meaning that the sections were being correctly merged, but the values within each section were just being replaced with the values present in the highest priority config item in the stack, rather than being merged.This means that values were not correctly "propagating up" thorugh holes in the config. For example, if a value was not present in the database, attempts to retrieve config based on all sources would only return what was in the database, failing to populate any values present in other sources (such as the hardcoded defaults) when they were missing from the database.
In place of
update()this PR uses a recursive function found on StackOverflow. Since these config objects are small, this recursive solution should be an acceptable tradeoff. If the config objects start getting really big and/or really nested (hundreds of items) then maybe it should be re-considered, but this scenario doesnt seem especially likely.This PR fixes #3476
Notes for Reviewers
This was tested with a unit test. to run it without causing a ton of errors from all the tests that still have failing database fixtures, run
uv run pytest tests/test_classes/. The unit test is written to purposely not use the database or any database mocks or fixtures so it can be run in more situations (hopefully making the tests easier to run even if they break again in future)Signed commits