(Work-in-progress) Update config file reading and validation #1694

flyinghyrax · 2024-03-29T01:18:56Z

Goals

Consolidate options from the mirror section into a type-annotated class
- This expands on the existing SetConfigValues named tuple
- Probably dataclass or attrs (I <3 attrs)
Consolidate reading and validation of mirror options into the configuration module
- Currently some options are read in validate_config_values and some in the mirror subcommand itself, we can move these into a single place
- Similarly with validation, some validation is performed e.g. inside the Mirror/Master classes, we can validate configuration at the program boundary so internal classes can assume configuration is valid
Define default values for all mirror options that are supposed to have one in a single location (Many options do not have a configured fallback value even if the documentation says so #990)
- Defaults should not have to be provided where an option is read, since this leads to defaults being set for some subcommands but not others
Change BandersnatchConfig into a regular class instead of a singleton
- The config object is already being passed as a parameter from main to async_main to subcommands
- " " " " " " " to the storage plugin initializers
- The config object is not being passed to filter plugin initializers, but there is a clear place to do so
- If the config object is injected as a parameter everywhere it is used, then a Singleton metaclass is unnecessary and all the unit tests become a bit simpler
(If needed) support old and new config implementations to coexist and be used simultaneously so modules can be migrated over time
Fix Add SOCKS support to proxy configuration parameter #1674 while we're in here

In Progress

A MirrorConfig attrs class
- I've got most of an implementation for this, but intend to capture some design notes regarding the approach I ended up taking
Update subcommands to to read values from validated class fields instead of directly from a ConfigParser instance
...

Completed

Fix evaluation of custom option reference syntax in diff-file
Fix a diff file being written unconditionally
Change storage plugin init to require an external ConfigParser instance
Change filter plugin init to require " " " "

codecov · 2024-03-29T01:20:59Z

Codecov Report

Attention: Patch coverage is 94.92188% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 84.32%. Comparing base (4d020e8) to head (0030683).
Report is 58 commits behind head on main.

❗ Current head 0030683 differs from pull request most recent head df9d88a. Consider uploading reports for the commit df9d88a to get more accurate results

Files	Patch %	Lines
src/bandersnatch/config/mirror_options.py	87.14%	7 Missing and 2 partials ⚠️
src/bandersnatch/mirror.py	75.00%	4 Missing and 3 partials ⚠️
src/bandersnatch/config/comparison_method.py	80.00%	3 Missing ⚠️
src/bandersnatch/configuration.py	62.50%	3 Missing ⚠️
src/bandersnatch/config/core.py	96.15%	0 Missing and 1 partial ⚠️
src/bandersnatch/main.py	0.00%	1 Missing ⚠️
src/bandersnatch/tests/test_mirror.py	98.71%	0 Missing and 1 partial ⚠️
src/bandersnatch/verify.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1694      +/-   ##
==========================================
+ Coverage   79.69%   84.32%   +4.62%     
==========================================
  Files          31       40       +9     
  Lines        4324     4574     +250     
  Branches      780      800      +20     
==========================================
+ Hits         3446     3857     +411     
+ Misses        721      533     -188     
- Partials      157      184      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/bandersnatch/tests/test_mirror.py

src/bandersnatch/tests/plugins/test_storage_plugin_s3.py

This replaces the implementation for custom configparser option references in the diff-file option. Moves detection and handling for the custom syntax into a dedicated submodule. Uses a regular expression to preserve text before and after the reference.

Restore original behavior where a diff file is only written if the 'mirror.diff-file' config option is set to a non-empty value.

Change the Storage base class and the storage plugin loading functions to require a ConfigParser instance so the BandersnatchConfig singleton is not accessed. Change the BandersnatchMirror class to require a Storage instance instead of a storage backend name since the mirror subcommand already loads a storage plugin instance. The diff in the application code is relatively simple, but both changes caused cascading/rippling updates to the unit tests that will require a regrettable amount of tedious review. It would be nice to find a way to split up this commit.

Change the Filter base class and filter loader to require an external ConfigParser instance be passed in. Change the Mirror/BandersnatchMirror classes to require an external instance of LoadedFilters. op/ed: Remember all that nonsense you told yourself about removing a singleton making the tests simpler? Well so far that is not the case.

flyinghyrax · 2024-04-08T00:51:16Z

...ran head-first into a good reason why configparser.ExtendedInterpolation was not already being used: regular expressions also use the $ character

c:\Users\mrsei\source\bandersnatch\src\bandersnatch\tests\plugins\test_regex_name.py::TestRegexProjectFilter::test_plugin_check_match failed: self = <test_regex_name.TestRegexProjectFilter testMethod=test_plugin_check_match>

    def test_plugin_check_match(self) -> None:
        bc = mock_config(self.config_contents)
    
>       mirror = make_test_mirror(config=bc)

src\bandersnatch\tests\plugins\test_regex_name.py:98: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src\bandersnatch\tests\plugins\util.py:27: in make_test_mirror
    local_config.read_dict(config)
...
    self._interpolate_some(parser, option, L, value, section, defaults, 1)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <configparser.ExtendedInterpolation object at 0x00000232263DFF90>
parser = <bandersnatch.config.core.BandersnatchConfig object at 0x00000232263DED10>
option = 'packages', accum = ['\n.+-evil'], rest = '$\n.+-neutral$'
section = 'filter_regex'
map = ChainMap({}, {'packages': '\n.+-evil$\n.+-neutral$'}, {}), depth = 1

    def _interpolate_some(self, parser, option, accum, rest, section, map,
                          depth):
        ...
        while rest:
            ...
            if c == "$":
                ...
            elif c == "{":
                ...
            else:
>               raise InterpolationSyntaxError(
                    option, section,
                    "'$' must be followed by '$' or '{', "
                    "found: %r" % (rest,))
E               configparser.InterpolationSyntaxError: '$' must be followed by '$' or '{', found: '$\n.+-neutral$'

C:\Program Files\Python311\Lib\configparser.py:517: InterpolationSyntaxError

...these changes are getting large enough that I wonder if I need a rethink here.

flyinghyrax force-pushed the config-updates branch from e891a9a to 1c833a2 Compare March 30, 2024 20:15

flyinghyrax commented Mar 30, 2024

View reviewed changes

src/bandersnatch/tests/test_mirror.py Outdated Show resolved Hide resolved

flyinghyrax commented Mar 30, 2024

View reviewed changes

src/bandersnatch/tests/plugins/test_storage_plugin_s3.py Outdated Show resolved Hide resolved

flyinghyrax added 7 commits April 1, 2024 18:46

Fix diff-file being written unconditionally.

7784f06

Restore original behavior where a diff file is only written if the 'mirror.diff-file' config option is set to a non-empty value.

Add changelog entry

b3b23b0

Change BandersnatchConfig into a regular class

eadc324

Add alternative config and mirror options classes

9e6244f

flyinghyrax force-pushed the config-updates branch from 64112eb to 9e6244f Compare April 6, 2024 21:49

Add inline backports of Self type for Python 3.10

0030683

Realized why ${} interpolation wasn't turned on

df9d88a

cooperlees mentioned this pull request Apr 12, 2024

configparser.NoOptionError: No option 'storage-backend' in section: 'mirror' #1702

Open

flyinghyrax closed this Apr 26, 2024

flyinghyrax deleted the config-updates branch April 26, 2024 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Work-in-progress) Update config file reading and validation #1694

(Work-in-progress) Update config file reading and validation #1694

flyinghyrax commented Mar 29, 2024 •

edited

codecov bot commented Mar 29, 2024 •

edited

flyinghyrax commented Apr 8, 2024 •

edited

(Work-in-progress) Update config file reading and validation #1694

(Work-in-progress) Update config file reading and validation #1694

Conversation

flyinghyrax commented Mar 29, 2024 • edited

Goals

In Progress

Completed

codecov bot commented Mar 29, 2024 • edited

Codecov Report

flyinghyrax commented Apr 8, 2024 • edited

flyinghyrax commented Mar 29, 2024 •

edited

codecov bot commented Mar 29, 2024 •

edited

flyinghyrax commented Apr 8, 2024 •

edited