Fix check of app.routes in app/config.py

[rknop]

As is, it would object that app.routes had changed from the default config file if you added something else from app.* (e.g. app.secret_key) to the config file without also copying app.routes from the default config file to the config file. Modify ensure_yaml_routes_matches_defaults in app/config.py to just check app.routes and make sure they're either not present, or not changed from the defaulits.

[pep8speaks]

Hello @rknop! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file app/config.py:

Line 21:9: E201 whitespace after '('
Line 21:34: E202 whitespace before ')'
Line 21:42: E201 whitespace after '('
Line 21:77: E202 whitespace before ')'
Line 23:80: E501 line too long (80 > 79 characters)
Line 24:31: E201 whitespace after '('

Comment last updated at 2022-01-05 20:39:53 UTC

[stefanv]

Thanks @rknop!

@acrellin @dmitryduev Do you know why this is necessary in baselayer? I don't see why the config.yaml.defaults and config.yaml have to match.

[acrellin]

Thanks @rknop!
I'm okay with this change. @stefanv We had previously been checking to ensure that there was no difference between app.routes in config.yaml and the defaults to avoid webpack errors related to missing components. This proposed change simply skips that check if app.routes is missing entirely in config.yaml (defaulting to the values in defaults makes sense here).

[stefanv]

I agree that the PR is correct, but as far as the motivation for the original change we may want to document it at the top of the config file so that users know they have to reproduce that section exactly.

@rknop We are using Python 3.8, so if you want to use that syntax you are welcome.

[dmitryduev]

@stefanv the reasoning for this was to save us from silently but constantly shooting ourselves in the foot when deploying new versions of Fritz with new components - we forgot too many times to propagate the changes in the BL/SP default config into Fritz's.

[stefanv]

@dmitryduev It looks to me as though this only compares .default.yaml and .yaml — the way I'd expect. Because otherwise you would not be able to extend the baselayer routes, which are very limited?

[dmitryduev]

ah, I see what you mean @stefanv. Should this be moved over to SP then?

[profjsb]

@rknop do we still need this PR or can it be closed?

[stefanv]

I've now removed the check in #279

If we keep running into issues with fritz, we can re-implement it there.