♻️✨ ⚠️ [DEVOPS] maintenance new settings (final round) #2836
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2836 +/- ##
========================================
+ Coverage 77.8% 79.1% +1.3%
========================================
Files 674 659 -15
Lines 27352 27181 -171
Branches 3182 3147 -35
========================================
+ Hits 21284 21510 +226
+ Misses 5322 4933 -389
+ Partials 746 738 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pcrespov
force-pushed
the
maintenance/new-settings-4
branch
from
db1c1b0 to
4b281f2
Compare
pcrespov
added
a:webserver
pcrespov
requested review from
sanderegg,
odeimaiz,
colinRawlings,
KZzizzle,
mguidon,
mrnicegyu11 and
Surfict
pcrespov
changed the title
| monkeypatch.setenv( | ||
| "RABBIT_PASSWORD", rabbit_settings.RABBIT_PASSWORD.get_secret_value() | ||
| ) | ||
| monkeypatch.setenv("RABBIT_CHANNELS", json.dumps(rabbit_settings.RABBIT_CHANNELS)) |
There was a problem hiding this comment.
minor: are these monkeypatch necessary? since the rabbit_settings are already setting them? but the port ok.
There was a problem hiding this comment.
This was the design in place. The way I understood is:
- start stack and wait until is up
- wait until given service is responsive (e.g. redis, rabbit, etc)
- fill settings to create a client for that service
and then, there are two types of fixtures
*_serviceguarantees services is up and running, and env vars to use it are in monkeypatched*_clientguarantees services is up and running, and returns a client ready to interact with it
packages/service-library/src/servicelib/aiohttp/application_setup.py
Outdated
Show resolved
Hide resolved
| class URLPart(Enum): | ||
| EXCLUDE = auto() | ||
| OPTIONAL = auto() | ||
| REQUIRED = auto() |
There was a problem hiding this comment.
just wondering about this auto method.. just checked in the documentation. how does that work if we add another value? it has to be appended right? at least if the value goes somewhere?
There was a problem hiding this comment.
it just adds a different value.
the way i use it is: if the enum is not going to be stored , then I just care that they are different and leave it to auto to choose them. If it is persisted, then I am the one settings the values explicitly
| # NOTE app_settings.WEBSERVER_STUDIES_ACCESS_ENABLED did not apply | ||
| "studies_dispatcher": {"enabled": app_settings.WEBSERVER_STUDIES_DISPATCHER}, | ||
| "studies_dispatcher": { |
There was a problem hiding this comment.
is this still needed without trafaret?
There was a problem hiding this comment.
yes. what i did was moving studis_access into studies_dispatch. These utils here is simply to convert the configs in the tests into env-vars. It is not used in the code. There is an explanation about these utils in the description above under the tests section.
…nd deprecates RedisConfig fixtures mdoels-library.settings.redis is removed
pcrespov
force-pushed
the
maintenance/new-settings-4
branch
from
62ce1fa to
9b0f81c
Compare
|
Note: This PR makes setting the Env-Var |
mrnicegyu11
changed the title
yes. @mrnicegyu11 check rules of the reverse proxy as well in case the |
What do these changes do?
This is the last of a series of PRs that progressively introduced the new-settings (i.e. moving from
trafaret-based config tosettings_library-based settings ) in the entire repo . Here is a summary of the PRs preceding this one:settings_librarywith new common settings and utilsweb-server: modules with dependencies totrafaretare isolated (e.g. splitting each plugin's configurations in_schema.py,_config.py).settings_library.base.BaseCustomSettingsweb-serverto use the newsettings_library. The trafaret-based config is marked as deprecated but is still used to run the app. We also introduced many runtime checks to ensure no configuration is lost with the new settings.Finally, this PR completely removes the deprecated trafaret-based config and activates the new settings to run the app 🎉 (long live to trafaret! it served us very well!). These changes also facilitate managing the plugin mechanism of the app using env vars. For that reason, all the plugins in the webserver were also carefully reviewed and reorganized ensuring at every step that the changes were covered by the tests.
In order to simplify the review, some descriptions of the changes and the ideas behind follow.
Changes in the
webserverThere are a lot of small changes throughout the webserver modules but they all follow certain guidelines. Here are the hightlights:
requirements:trafaretremovedconfigallyamlconfigurations removed*_schemaand*configmodules including all config dependencies (separated during PR ♻️ Maintenance/new settings (2nd round) #2739) removed. Replaced bysettingsApplicationSettings.WEBSERVER_MYPLUGIN. Note that the plugin can be disabled by simply settings the env varWEBSERVER_MYPLUGIN=null.@app_module_setupwhere it inits the plugin and "merges with theapp". Typical operations areapp.routesorapp.middlewaresappsignals (app.on_startup,app.cleanup_ctx, ...)app[MY_INSTANCE] = obj). E.g. creates clients and waits a particular service to connect.WEBSERVER_RESOURCE_MANAGERnow does not include settings ofWEBSERVER_GARBAGE_COLLECTORWEBSERVER_SCICRUNCHit's a plugin on its ownstudies_accessmodules ->studies_dispatcher(WEBSERVER_STUDIES_DISPATCHER)webserver/testswere carefully refactored side-by-side with the plugins. These are some of the criteria usedmyplugin, all associated tests must betest_myplugin*.pyregardless of the folder where it is.TestClient(typically denotedclient) and connects to aclient.appthat is configured in each case overridingapp_configfixture that would represent the trafaret-based config file. With the new settings design, the app is not configured anymore from file (i.e.app_config) but it uses instead env vars, as recommended in the 12-factor app. In order to overcome this change, we created some tools that would transformtrafaret-based config into env-vars. The latter are then mocked usingmonkeypatch.setenvto produce the same effect on the new app setting (seeapplication_settings_utils.pyand alsomonkeypatch_setenv_from_app_configfixture). This approach, not only saved us from a huge refactoring but also guarantees that theTestClient.appfixtures remain the same as they were before.pytest_simcore(reusability and reduce local module helpers)test/data(easily accessible usingtest_data_dirfixture)Changes outside the
webservermodels_librarymodels_library.settings.rabbitandmodels_library.settings.redisremoved (replaced by newsettings_librarycounterpartspytest_simcore:helpers:utils_dictminorutils_dockerfixes onsafe_docker_infosdue to long names produces when creatingtest_failuresreportsutils_login: adapted to new settings for login plugin. Removed global config. Further refactoring might be necessary to access users withaiopg. This is a very old fixtureutils_projectsmoved some of the helpers with projects here to reduce redundancy in project test suitesrabbit_servicefixtures to use newservice_settings.rabbitredis_servicefixtures to use newservice_settings.redisservicelibapplication_setupenhances plugin initialization. Now decorator relies on application settings (instead of configs) and ensures a that the plugin setup is only executed once. NOTE: @GitHK @sanderegg I will add the dependency feature we discussed in a separate PR.settings_libraryutils_service: simplified composing URLs from host, port ... (seetest_utils_service.py)utils_session: New mixin to check fernet keys. Used in both theweb-serverandapi-serversession settingsapi-serversettingsuses new mixin fromservicelib.utils_session.webservermodule adapted to use new settings . Fixes communication due to changes in webserverdirector-v2:/testsrefactored due to deprecation ofmodels_library.settings.rabbitandmodels_library.settings.redisdirector/testshighlighted in doc that changes inpytest_simcorethat are not python 3.6 compliant will break these tests (it happened during this PR and errors were quite cryptic)dynamic-sidecar/testssame as aboveREST_SWAGGER_API_DOC_ENABLEDnecessary if the swaggerAPI docs are to be exposedRelated issue/s
How to test
Take any plugin (e.g.
exporter), then typeNOTE: we could create a make recipe to test plugins
Checklist
activity/module_setup.py|pytest --pdb tests/unit/**/test_activ*.pycatalog.py|pytest --pdb -vv --ff tests/unit/**/test_catalog*.pyclusters/module_setup.py|pytest --pdb -vv tests/unit/with_dbs/**/test_clust*.pycomputation.pymake openapi-specs,git commit ...and thenmake version-*)cd packages/postgres-database,make setup-commit,sc-pg review -m "my changes"