Fix Inheritance-Break Validation Functionality #373 #385
Conversation
|
Still in the progress of reorganizing and adding test cases. |
Codecov Report
@@ Coverage Diff @@
## main #385 +/- ##
==========================================
- Coverage 80.85% 80.40% -0.46%
==========================================
Files 62 62
Lines 2387 2388 +1
==========================================
- Hits 1930 1920 -10
- Misses 457 468 +11
|
|
A lot of the changes were for the test cases. The "fix" is removing the check for group configs if the common config doesn't exist just to keep it simple for now. I did add one test case to ensure nothing is broken if the flag is set in the group and the common config doesn't exist. As mentioned previously in our Slack discussion, it will make our codebase more complicated to limit where the inheritance-break flag can be set. |
Fatal will exit the program anyway
By passing in additional parameters to the validation function users can better find where they set the invalid inheritance-break flag.
Merging configs is more costly performance-wise. If we can validate early then the merging doesn't have to run.
If common config and group config doesn't exist then we can use group_and_project to capture the full section name.
The invalid tests for projects and groups are added so far.
If the parent key doesn't exist then the inheritance-break flag is set outside a setting.
Tests are nested within classes. I personally think it just looks cleaner. Removed redundant test cases and settings within fixtures.
Added two more test cases with the flag set at the group level.
…l is the highest level Since the inheritance-break validation in groups was providing false errors, there needs to be a check to see if the merging breaks if there is nothing to inherit from.
|
I did this rebase because I have added updates to main branch to resolve the issue of the flaky tests, @ep-linden. |
A config is invalid if a flag is set at the common level.
Using those key names unnecessarily implied ordering.
|
I took the liberty to push a minor code deduplication into your branch, @ep-linden. Please check out the changes above. |
|
I also wanted to (arguably) improve the tests a bit with tdslinden#1, but the tests don't pass after such change. Can you take a look a that please, @ep-linden? I think that the change might be worth it and that maybe we have found a real issue here. |
tests/unit/configuration/test_inheritance_break_projects_and_groups.py
Outdated
Show resolved
Hide resolved
Change the debug statement accordingly.
Since group and subgroups configs will be processed through the get_effective_config_for_group function, tests were added to account for that. Clearly stated whether the test is using get_config_for_project or get_config_for_group.
Add another test case to show the usage of breaking the inheritance from the group settings. Modify the test cases to include group_settings
Logic is added to groups.py to validate group level config
No description provided.