Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ Maintenance/new settings (1st round) #2736

Merged
merged 28 commits into from
Jan 13, 2022

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jan 13, 2022

What do these changes do?

This is the first of a series of PR that will progressively introduce the new-settings (i.e. move from trafaret to settings_library ) in the entire repo. In this PR we address:

settings_library :

  • new settings for Prometheus and SMTP (Simple Mail Transfer Protocol)
  • new mixin that will be use in settings subclasses to configure services (typically with port, host, etc)
  • renaming of modules:
    • distinguish three groups:
      • base modules base prefix
      • settings groups (no prefix)
      • utils with mixins, ... utils_ prefix
      • internal like _constants

♻️ webserver

  • reducing coupling between plugins (typically by moving constants to its own module reduces significantly coupling)
  • see commits for details (looking at file changes only is not very informative)
  • renaming of modules:
    • settings -> application_settings since it goes side-by-side with application_* modules
    • constants -> _constants: to stress it is internal

NOTE: this PR is very "noisy" but in reality there are very few changes: see commits message to guide your review.

Related issue/s

How to test

webserver and settingslib tests covers changes

Checklist

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #2736 (d3b1726) into master (1f76deb) will increase coverage by 0.0%.
The diff coverage is 93.2%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2736   +/-   ##
======================================
  Coverage    78.2%   78.2%           
======================================
  Files         647     652    +5     
  Lines       26915   26963   +48     
  Branches     2617    2617           
======================================
+ Hits        21062   21106   +44     
- Misses       5144    5146    +2     
- Partials      709     711    +2     
Flag Coverage Δ
integrationtests 65.6% <89.4%> (-0.1%) ⬇️
unittests 74.0% <93.2%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...src/servicelib/aiohttp/web_exceptions_extension.py 0.0% <0.0%> (ø)
...ings-library/src/settings_library/utils_logging.py 87.5% <ø> (ø)
...s/api-server/src/simcore_service_api_server/cli.py 0.0% <0.0%> (ø)
...ore_service_director_v2/utils/client_decorators.py 69.2% <ø> (ø)
...imcore_service_webserver/meta_modeling_handlers.py 73.0% <28.5%> (ø)
...ettings-library/src/settings_library/prometheus.py 91.6% <91.6%> (ø)
...ages/models-library/src/models_library/services.py 100.0% <100.0%> (ø)
...library/src/servicelib/aiohttp/application_keys.py 100.0% <100.0%> (ø)
...ettings-library/src/settings_library/_constants.py 100.0% <100.0%> (ø)
...ttings-library/src/settings_library/basic_types.py 100.0% <100.0%> (ø)
... and 54 more

@pcrespov pcrespov self-assigned this Jan 13, 2022
@pcrespov pcrespov added this to the Rudolph milestone Jan 13, 2022
@pcrespov pcrespov changed the title WIP: Maintenance/new settings 1 ♻️ Maintenance/new settings 1 Jan 13, 2022
@pcrespov pcrespov added a:webserver issue related to the webserver service t:maintenance Some planned maintenance work labels Jan 13, 2022
@pcrespov pcrespov marked this pull request as ready for review January 13, 2022 09:37
@pcrespov pcrespov changed the title ♻️ Maintenance/new settings 1 ♻️ Maintenance/new settings (1st round) Jan 13, 2022
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! please have a look at my comments before merging

@@ -2,7 +2,7 @@
from pydantic.tools import parse_raw_as
from settings_library.base import BaseCustomSettings

from .constants import GB
from ._constants import GB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there a _ in front of the module name? it cannot be re-used outside of the library? stuff like GB is not really something private is it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a way to say to express what is part of the API of the library and what is not
the constants, as you mention, are not too troublesome regarding coupling BUT this way I avoid having the services importing the same constant from different libraries. That is the ida

packages/settings-library/src/settings_library/email.py Outdated Show resolved Hide resolved
packages/settings-library/src/settings_library/email.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants