feat: factory settings#925
Conversation
chaen
force-pushed
the
feat_factory_settings
branch
from
aedbf8a to
18d7e66
Compare
chaen
force-pushed
the
feat_factory_settings
branch
from
d7b1d17 to
29c5f90
Compare
chaen
force-pushed
the
feat_factory_settings
branch
5 times, most recently
from
d9f369d to
427a9ef
Compare
| in FactorySettings, which reads from environment variables | ||
| prefixed with ``DIRACX_DB_URL_{DB_NAME}``. | ||
| """ | ||
| from diracx.core.settings import FactorySettings |
There was a problem hiding this comment.
Does this need to be protected?
There was a problem hiding this comment.
I think not, I'll try
|
|
||
| DEFAULT_REDIS_URL = "redis://localhost" | ||
| REDIS_URL_ENV_VAR = "DIRACX_TASKS_REDIS_URL" | ||
| _factory_settings = FactorySettings() |
There was a problem hiding this comment.
Why is this one global but the other use cases aren't. We should decide one or the other.
I wonder if a global instance in settings.py makes more sense...
There was a problem hiding this comment.
Because we tried hard to avoid global variable. I did not want to introduce any.
This one here is a bit special in the sense that this file is pretty much a script so I felt like it was okay.
| type=str, | ||
| default=None, | ||
| help=f"Redis URL (default: ${REDIS_URL_ENV_VAR} or {DEFAULT_REDIS_URL})", | ||
| help=f"Redis URL (default: ${{REDIS_URL_ENV_VAR}} or {DEFAULT_REDIS_URL})", |
| type=str, | ||
| default=None, | ||
| help=f"Redis URL (default: ${REDIS_URL_ENV_VAR} or {DEFAULT_REDIS_URL})", | ||
| help=f"Redis URL (default: ${{REDIS_URL_ENV_VAR}} or {DEFAULT_REDIS_URL})", |
There was a problem hiding this comment.
toto = "abc"; print(f"a literal for the environment variable ${{toto}} while here I want the value of a python variable {toto=}" )
a literal for the environment variable ${toto} while here I want the value of a python variable toto='abc'| @pytest.fixture(scope="session") | ||
| def test_factory_settings() -> Generator[FactorySettings, None, None]: | ||
|
|
||
| from diracx.core.settings import FactorySettings |
There was a problem hiding this comment.
Does this need to be protected?
There was a problem hiding this comment.
I think here I just blindly reapplied the same pattern as the other fixtures.
| settings = AuthSettings() | ||
| db_url = os.environ["DIRACX_DB_URL_AUTHDB"] | ||
| factory_settings = FactorySettings() | ||
| db_url = factory_settings.sql_dbs.AuthDB |
There was a problem hiding this comment.
| db_url = factory_settings.sql_dbs.AuthDB | |
| db_url = factory_settings.sql_dbs["AuthDB"] |
There was a problem hiding this comment.
Hum, I wonder why this one was not spotted by the static analyzer 🤔
| # Try to connect to Redis for lock acquisition | ||
| redis: LockCoordinator | None = None | ||
| redis_url = os.environ.get(REDIS_URL_ENV_VAR) | ||
| redis_url = _factory_settings.tasks_redis_url |
There was a problem hiding this comment.
This used to be optional now if redis_url: is always true.
I haven't checked in detail to figure out what the correct behavoir is
There was a problem hiding this comment.
I believe it was always needed
| use_attribute_docstrings=True, validate_by_alias=True, validate_by_name=True | ||
| ) | ||
|
|
||
| config_backend_url: ConfigSourceUrl | None = Field( |
There was a problem hiding this comment.
This used to be mandatory, maybe it still should be?
There was a problem hiding this comment.
i made it non mandatory because of things like the OpenSearch init jobs that do not need that
chaen
force-pushed
the
feat_factory_settings
branch
from
427a9ef to
16c70c9
Compare
2 participants
Closes #894
Introduce FactorySettings to centralize shared global configuration that does not belong to a single service settings class.
Changes:
In principle, that change should be transparent